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

Edition should not start capitalize letter #6149

Merged
merged 13 commits into from
Mar 29, 2020
Merged

Edition should not start capitalize letter #6149

merged 13 commits into from
Mar 29, 2020

Conversation

fabgio
Copy link
Contributor

@fabgio fabgio commented Mar 20, 2020

  • Add EditionChecker feature to permit edition to begin with a number
  • Add proper test
  • Update CHANGELOG

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Codewise this is fine so far, but you somehow have the wrong code style.
To avoid conflicts due to different code styles, we have created a custom code style for formatting.
See here for details:
https://github.com/JabRef/jabref/tree/master/config

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Thank you for the contribution and welcome to JabRef 🎉

I have some "nitpick" comments. Some of them seem to be caused, because the steps we wrote at https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace#configure-your-ide were not fully followed. Can we do something to improve the howto?

fabgio added 2 commits March 22, 2020 17:16
Remove space
Remove space
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 more comments 😇

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Please do not remove the predefined RunConfiguration "JabRef_Main.xml".

@fabgio fabgio closed this Mar 23, 2020
@fabgio fabgio reopened this Mar 23, 2020
@fabgio
Copy link
Contributor Author

fabgio commented Mar 23, 2020

Did all my best!

@calixtus
Copy link
Member

Hi @fabgio, great work so far. I still see some problems with checkstyle.
A simple trick: If you use IntelliJ with CheckStyle, you can simply select everything and push Ctrl+Alt+L and mosts or all identation and checkstyle errors should be gone.

Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

So beautiful. ;-) LGTM.

@fabgio
Copy link
Contributor Author

fabgio commented Mar 27, 2020

I made all the improvments you suggest. i also semplified a nested if condition in EditionChecker class

@fabgio fabgio closed this Mar 27, 2020
@fabgio fabgio reopened this Mar 27, 2020
@Siedlerchr
Copy link
Member

Codewise looks good to me! You still have some checkstyle issues, please resolve them so we can merge Just use reformat code Ctrl+Alt+L in the file

@koppor koppor merged commit 0045e84 into JabRef:master Mar 29, 2020
@koppor
Copy link
Member

koppor commented Mar 29, 2020

Follow ups:

@fabgio Thank you for the PR. Looking forward to more contributions. In case you are more keen to write some helping text, you are invited to join our efforts on improving the documetnation: https://github.com/JabRef/user-documentation/issues

@JabRef JabRef deleted a comment from codecov bot Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants