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

Removed a number of warnings, added copyright etc #266

Merged
merged 2 commits into from
Oct 29, 2015

Conversation

oscargus
Copy link
Contributor

Cleanups based on warnings, primarily:

  • Removed right-hand side arguments of diamond operators
  • Static functions
  • Commented empty blocks
  • Added try-with
  • Avoided hiding variables

Included copyright statements.

@@ -127,18 +117,14 @@ public void databaseChanged(DatabaseChangeEvent e) {
break;
case CHANGED_ENTRY:
// Entry changed. Resort list:
//Collections.sort(set, comp);
Copy link
Member

Choose a reason for hiding this comment

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

Should the resort list comment be removed, too? Or is the implementation below some kind of resorting?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I interpreted the first line as a comment and the second as a comment-out. :-) But reading it, I see your point. Will fix.

@koppor
Copy link
Member

koppor commented Oct 27, 2015

Other than my minor comments: 👍

@oscargus
Copy link
Contributor Author

When I saw System.*.print* I replaced it with LOGGER, but I haven't searched for it (yet). But, yes, I can changed that.

OK, will try and look for those else if:s.

@oscargus
Copy link
Contributor Author

XMPUtil is a command line tool, so I think that it makes sense to keep it as is there?

@koppor
Copy link
Member

koppor commented Oct 28, 2015

I don't get the comment about XMPUtil. Isn't it just a plain class? May it also be used as executable? Even if, we should use logging, because we mainly use it in JabRef.

@matthiasgeiger
Copy link
Member

@koppor The only System.out.'s are in the main()-method of the class (and in a method which explains the usage() of the CLI mode. However, it seems to be that the possibility of the standalone usage is documented nowhere...

Remove it? Leave it as is? Or use a Logger even for the CLI infos?

@koppor
Copy link
Member

koppor commented Oct 28, 2015

Leave as is. Add a comment above the line (for our greppers). Add a note at our ticket to extract as external lib. That will be a standalone tool then! (lib+cmd using that lib).

@simonharrer
Copy link
Contributor

The main method (CLI) cannot be called at the moment. I checked 2.10 and we only shipped it without building an additional jar with a different main method. Why not just remove this then?

@koppor
Copy link
Member

koppor commented Oct 28, 2015

I believe, it might be a useful tool for some power users.

@matthiasgeiger
Copy link
Member

Maybe 😉
But wouldn't it be better to integrate it in the normal JabRef CLI?

@simonharrer
Copy link
Contributor

-> dev call :)

@simonharrer
Copy link
Contributor

Thank you @oscargus for this PR.

simonharrer added a commit that referenced this pull request Oct 29, 2015
Removed a number of warnings, added copyright etc
@simonharrer simonharrer merged commit bb7c0f2 into JabRef:master Oct 29, 2015
@oscargus oscargus deleted the warningremoval2 branch November 1, 2015 16:44
@koppor koppor mentioned this pull request Apr 27, 2017
koppor pushed a commit that referenced this pull request Oct 1, 2022
cb98d36691 copied .github/workflows/merge.yaml .github/workflows/sheldon.yaml from styles
62dcca65d7 Update locales-nn-NO.xml (#267)
eb8587463f Update locales-nb-NO.xml (#266)
bd502ffb0d Update locales-pt-PT.xml (#262)
2dcc82ced1 Update locales-it-IT.xml (#263)

git-subtree-dir: buildres/csl/csl-locales
git-subtree-split: cb98d366912a0c0be361880e12fbc43cef6d383e
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