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

A first solution for the cli problem #4038 #4047

Merged
merged 2 commits into from
May 24, 2018

Conversation

johannes-manner
Copy link
Collaborator

@johannes-manner johannes-manner commented May 22, 2018

ExportFactory is not initialized at this execution point...
The printUsage method is used twice. The JabRefCLI prints the usage, if an error occurs during argument parsing and the ArgumentProcessor prints the usage, when --help option is chosen.

I made two solutions for this problem:

  • Commit 1: Some sort of a hack to only get the names of the exporters without altering the processing in the main method.
    image

  • Commit 2: Changed the order of execution in the main method. The argument processor needs the export factory. I don't know, if the processing of the preferences will influence the cli functionality?

Fixes #4038

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef

ExportFactory is not initialized at this execution point...
@johannes-manner johannes-manner changed the title A first solution for the cli problem A first solution for the cli problem #4038 May 22, 2018
@johannes-manner johannes-manner added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 22, 2018
@stefan-kolb
Copy link
Member

TL;DR: I guess we should use the second method. Let's see what the others think.

I guess the second solution is fine, if not, we will find out.
I think we could also extract some methods here for the initialization of the migrations, and then the preferences from the start method and afterwards instantiate the argument processor so the start method gets more clear.

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.

Yes, the second solution looks better.

@tobiasdiez tobiasdiez merged commit 7671b80 into JabRef:master May 24, 2018
@stefan-kolb stefan-kolb mentioned this pull request May 24, 2018
Siedlerchr added a commit that referenced this pull request May 27, 2018
…leType

* upstream/master: (33 commits)
  Import inspection uses now same font size setttings as maintable (#4062)
  Add date checker (#4007)
  Enable tests
  macOs push to application fix
  Fix #4041 to make Push to Application work again on OSX. (#4057)
  Add NormalizeEnDashesFormatter (#4045)
  Package private for tests
  Shutdown duplicate code
  Move migrations to PreferencesMigrations
  Structure startup
  Extract migrations
  A first solution for the cli problem #4038 (#4047)
  New translations JabRef_en.properties (French) (#4052)
  Update CHANGELOG.md
  Fix checkstyle
  Make test more informative in the failing case
  New translations JabRef_en.properties (French) (#4051)
  New translations JabRef_en.properties (Vietnamese) (#4050)
  Make Formatter an abstract class (and remove AbstractFormatter)
  New Crowdin translations (#4048)
  ...

# Conflicts:
#	CHANGELOG.md
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