jabref icon indicating copy to clipboard operation
jabref copied to clipboard

Introduce formatter to remove word-enclosing braces

Open rohit-garga opened this issue 10 months ago • 7 comments

Closes #11222

Mandatory checks

  • [ ] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • [x] Tests created for changes (if applicable)
  • [x] Manually tested changed features in running JabRef (always required)
  • [ ] Screenshots added in PR description (for UI changes)
  • [ ] Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • [ ] 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.

rohit-garga avatar Apr 26 '24 13:04 rohit-garga

It's not clear to me why fetcher tests are failing, they failed even after I reverted my changes. Can you help please @koppor

rohit-garga avatar Apr 26 '24 13:04 rohit-garga

You can ignore the failing fetcher tests. They often have problems due to network errors or changed data. Only relevant if you changed soemthing at the Fetchers itself

Siedlerchr avatar Apr 26 '24 13:04 Siedlerchr

@rohit-garga Due to the nature of tests, it is not easy to run all. Sorry for that.

Partially running works.

E.g., all tests in logic

image

Then select "test"

image

Also works for architecture and model.

Does not work for gui. There, you need to choose guiTest.

koppor avatar Apr 26 '24 15:04 koppor

Understood, thanks! Let me know when you get chance to review :)

rohit-garga avatar Apr 26 '24 16:04 rohit-garga

I have only small comments, but explaining these was more hard than "just doing" it. -- I had no write rights to fix minor issues. Thus, I filed a PR: https://github.com/rohit-garga/jabref/pull/1

koppor avatar Apr 29 '24 08:04 koppor

Hi @koppor , I need some help-

1.) We changed the formatter to return the string value instead of null. This does not work, because in the test case, we pass an empty string in the constructor, but a parser will get null not an empty string. For example

PersonNameSuggestionProviderTest.completeLowercaseBeginningOfNameReturnsName Expected :[Author{givenName='Vassilis', givenNameAbbreviated='V.', namePrefix='', familyName='Kostakos', nameSuffix=''}] Actual :[Author{givenName='Vassilis', givenNameAbbreviated='V.', namePrefix='null', familyName='Kostakos', nameSuffix='null'}]

2.)

FormatterTest.formatOfNullThrowsException

All formatters need to throw a null pointer exception when we run format(null). In our test cases, there are many instances of tests where we pass null in the constructor for Author.java.

One easy solution would be 1.) Return null whenever we get an empty string/ null. 2.) We add a try-catch in the author constructor, so we can catch null pointer exceptions and continue.

rohit-garga avatar May 04 '24 10:05 rohit-garga

@rohit-garga Step 1 is OK. Step 2: Use an if instead of try/catch, because control flow should not be done using exceptions 😅

koppor avatar May 04 '24 11:05 koppor

@rohit-garga Thank you for the follow-ups. I think, the null handling is now OK. Someone touching the code later will see. 😅

koppor avatar May 13 '24 09:05 koppor