Skip to content

Globals.pref dependency in BibTexParserTest - KeyPattern #1843

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

Closed
Siedlerchr opened this issue Aug 24, 2016 · 9 comments
Closed

Globals.pref dependency in BibTexParserTest - KeyPattern #1843

Siedlerchr opened this issue Aug 24, 2016 · 9 comments

Comments

@Siedlerchr
Copy link
Member

Just noticed one more Globals.prefs dependency: @oscargus Do you want to take care of it?
in BibTexParserTest in integrationTestCustomKeyPattern

     AbstractBibtexKeyPattern bibtexKeyPattern = result.getMetaData()
                .getBibtexKeyPattern(Globals.prefs.getKeyPattern());

AbstractBibtexKeyPattern expectedPattern = new DatabaseBibtexKeyPattern(Globals.prefs.getKeyPattern());
@oscargus
Copy link
Contributor

If I recall correctly something else will break if one removed the Globals.prefs initialization. I think it is that ParserResults creates a MetaData() object which is not free from Globals (yet). Nowadays I try to add some comments to the setUp-method of the test to motivate why Globals.prefs is still initialized...

@oscargus
Copy link
Contributor

So even if you can write JabRefPreferences.getInstance().getKeyPattern() there you still need the Globals.prefs init in setUp. The only remaining Globals in MetaData is the keyword separator which is needed for the groups. However, I haven't really found a nice way to propagate it (and I'm pondering if I should just propagate that as a String or create a GroupsPreferences class only containing this field...).

@oscargus
Copy link
Contributor

And feel free to take care of it if you like. I will probably do it eventually otherwise, but not in the near future as it feels right now.

@Siedlerchr
Copy link
Member Author

Ah okay thanks for the info, I will check if it works with getInstance() otherwise I will add a comment.

@oscargus
Copy link
Contributor

One way may be to assign a private JabRefPreferences field as there are quite a few tests that need an instance. But it may not be enough and Globals.prefs is still needed. Excellent if you want to give it a try!

@oscargus
Copy link
Contributor

BibDatabaseContext is the major obstacle for Globals free logic-tests btw. If that is imported one can just forget about it... And I have no idea how to clean it in a simple way as it relies quite heavily on it.

@lenhard
Copy link
Member

lenhard commented Aug 26, 2016

In a perfect world (that is: JabRef architecture), BibDatabaseContext should be in model. Given its current coupling to global classes, this is illusive, however.

@oscargus
Copy link
Contributor

With #1856 it is actually not so illusive anymore. One dependency left in MetaData and then we can move MetaData and BibDatabaseContext.

@oscargus
Copy link
Contributor

I close this as recents PRs have removed the need for Globals in all but one logic test (CleanupWorkerTest).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants