Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default behavior of resolve bibtex strings #8382

Merged
merged 33 commits into from
Jan 13, 2022
Merged

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Jan 3, 2022

Fixes #7010
Fixes #7012
Fixes #8303

grafik

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 3, 2022
@Siedlerchr
Copy link
Member Author

Siedlerchr commented Jan 3, 2022

Discussion: Invert logic And resolve fields only for the following list:
Author, BookAuthor, Title, Editor , Journal + JournalTitle + SubTitile etc
Editor a, b,c Issue + SubTitle, MainTitle, MainSubTitle, MainTitleAddon, Publisher, shortAuthor + Editor + Title

TODO: Double braces in Changelog

@Siedlerchr Siedlerchr removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 3, 2022
@Siedlerchr Siedlerchr marked this pull request as draft January 3, 2022 23:04
* upstream/main:
  Observable Preferences J (Preview) and minor refactor (#8370)
  quickfix (#8383)
  Live reloading when switching themes (#7336)
@Siedlerchr
Copy link
Member Author

add field month to string resolving

@Siedlerchr Siedlerchr marked this pull request as ready for review January 6, 2022 20:15
@Siedlerchr Siedlerchr requested a review from koppor January 6, 2022 21:28
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 7, 2022
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nitpick regarding the negation - and two additional fields. 😇

Siedlerchr and others added 4 commits January 8, 2022 18:51
* 'main' of github.com:JabRef/jabref:
  Replace master with main
…gResolving

* 'stringResolving' of github.com:JabRef/jabref:
  Fix typos
@koppor koppor marked this pull request as draft January 9, 2022 09:46
@koppor
Copy link
Member

koppor commented Jan 9, 2022

I worked on the preferences tab to improve the UI by grouping the preferences for "File" together:

grafik

@koppor
Copy link
Member

koppor commented Jan 9, 2022

I would now like to add test cases for the fields mentioned in the fixed issues. I know, It's "only" a preference (mocking) thing. However, I would like to have more tests to ensure that nothing breaks if we touch this part of the code at a later time.

@calixtus
Copy link
Member

calixtus commented Jan 9, 2022

Do you want to test the FieldWriter with the mentioned fields in this PR or do you want to test the preferences if they return the right fields? I don't think the latter would be meaningful at all...

Btw., this PR is still a draft...

@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 9, 2022
@Siedlerchr
Copy link
Member Author

failing fetcher test is grobid

@Siedlerchr Siedlerchr marked this pull request as ready for review January 9, 2022 18:36
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 9, 2022
@koppor
Copy link
Member

koppor commented Jan 9, 2022

I added the BibTeX field content of the three linked issues as tests (verbatimely).

@Siedlerchr
Copy link
Member Author

I am not sure if your negation change doesn't break anything

@koppor
Copy link
Member

koppor commented Jan 10, 2022

Side note: I find isEnabled more readable than !isDisabled. Pattern "Avoid Negations", page 4, in Java by Comparison.

I am not sure if your negation change doesn't break anything

  • Tests run through :) -- In case one has a gut feeling something could break, please provide examples, and I am happy to craft tests.
  • I replace the negative clause by positive one and adding !.
  • All positive parameters (true) have been replaced by negative ones (false) and vice versa
  • "in a distributive lattice, an element has, at most, one complement" (https://math.stackexchange.com/a/3439133/75732)

@koppor
Copy link
Member

koppor commented Jan 10, 2022

The only thing, I did not touch is the storing in the registry. There, the negegation is still stored.

@@ -163,7 +168,7 @@ private String formatAndResolveStrings(String content, Field field) throws Inval
}

private boolean shouldResolveStrings(Field field) {
if (!preferences.isDoNotResolveStrings()) {
if (preferences.isResolveStrings()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldResolveStrings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up PR 😇

@Siedlerchr
Copy link
Member Author

After looking again at the changes, it looks good and I will merge it now

@Siedlerchr Siedlerchr merged commit ccf20b4 into main Jan 13, 2022
@Siedlerchr Siedlerchr deleted the stringResolving branch January 13, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
4 participants