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

Fix bracket collisions #6469

Merged
merged 4 commits into from
May 12, 2020
Merged

Fix bracket collisions #6469

merged 4 commits into from
May 12, 2020

Conversation

btut
Copy link
Contributor

@btut btut commented May 12, 2020

Brackets in regular expressions were interpreted as brackets for fields. This change allows users to define regular expressions that contain brackets. This led to an exception before.

  • Change in CHANGELOG.md described (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 created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@btut btut mentioned this pull request May 12, 2020
5 tasks
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. I've one question, where I'm not sure that a certain edge case is currently covered correctly. Apart from this 👍 for merge.

Boolean foundClosingBracket = false;
// make sure to read until the next ']'
while (st.hasMoreTokens()) {
String subtoken = st.nextToken();
Copy link
Member

Choose a reason for hiding this comment

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

Question: you fetch the next token above in line 131, and then here the subtoken. So this moves the stream forward twice, right? What happens if the user adds an empty brace, i.e []. Is the closing bracket still found? Can you please add a test for this case as well. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! But what should "[]" expand to? Inside a quote, the parsing should work fine. but what does "[]" mean in terms of JabRef fields? Should it just do nothing and print an appropriate warning?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would say it should do nothing and log a warning. I don't really see a use case for the empty bracket, it should just not break the parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 230fef2

@tobiasdiez tobiasdiez added status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 12, 2020
@tobiasdiez tobiasdiez removed the status: changes required Pull requests that are not yet complete label May 12, 2020
@Siedlerchr Siedlerchr merged commit ce49261 into JabRef:master May 12, 2020
@Siedlerchr
Copy link
Member

Thanks again for your valuable contributions! 🥇

@btut
Copy link
Contributor Author

btut commented May 13, 2020

Glad to help and happy to have my regular expressions supported :)

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
Development

Successfully merging this pull request may close these issues.

3 participants