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.

Sign up for GitHub

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

Jump to bottom

Replace new instantiation of BuildInfo with calls to Injector #11954

Merged
merged 11 commits into from
Oct 14, 2024
Merged

Replace new instantiation of BuildInfo with calls to Injector #11954

merged 11 commits into from
Oct 14, 2024

Conversation

calixtus
Copy link
Member

@calixtus calixtus commented Oct 14, 2024 •
edited
Loading

closes https://github.com/JabRef/jabref-issue-melting-pot/issues/604

also removes some telemetry artifacts

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@calixtus calixtus added the type: code-quality Issues related to code or architecture decisions label Oct 14, 2024
@calixtus calixtus requested a review from koppor October 14, 2024 07:33
koppor
koppor previously approved these changes Oct 14, 2024
View reviewed changes
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.

Looks good - you decided against constructor-based injection, but sued the Injector directly? If it is OK for you, it is OK for me!

@calixtus
Copy link
Member Author

I have no hard opinion on this atm. Probably think about some hours. Will come back to this later

@koppor
Copy link
Member

koppor commented Oct 14, 2024

I think, constructor-based injection will increase the code, but the dependencies are explicit. In constract, with the injector, one "just" needs to search for class usages with an Injector...

Using the example of BuildInfo can guide us to fix an ADR. -- I mean: Implement with Injector (done here), implement as constructor passing (maybe in another PR) and then compare.

@Siedlerchr
Copy link
Member

In general, constructor based dependency injection is preferable because you can better test it and it's more explicit. For example, spring also recommends ctor based injection

@github-actions GitHub Actions
Copy link
Contributor

The build of this PR is available at https://builds.jabref.org/pull/11954/merge.

koppor
koppor requested changes Oct 14, 2024
View reviewed changes
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.

I like keeping the preferences together. - Some code still needs to be adapted to use

importerPreferences.getApiKey(getName())

instead of

Injector.instantiateModelOrService(BuildInfo.class).semanticScholarApiKey

because the keys are now provided by importerPreferences.getApiKey

...a/org/jabref/gui/entryeditor/citationrelationtab/semanticscholar/SemanticScholarFetcher.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/logic/preferences/JabRefCliPreferences.java Show resolved Hide resolved
docs/code-howtos/fetchers.md Outdated Show resolved Hide resolved
@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 14, 2024
@InAnYan
Copy link
Collaborator

InAnYan commented Oct 14, 2024

#11954 (comment)

Then what is the purpose of DI framework? Like the one we use?

@calixtus
Copy link
Member Author

Afterburner is not a di framework.
It just provides some basic functionality to provide injections into presenters.
See discussion here: #11342 (comment)

@koppor koppor enabled auto-merge October 14, 2024 20:36
@koppor koppor added this pull request to the merge queue Oct 14, 2024
@github-actions GitHub Actions
Copy link
Contributor

The build of this PR is available at https://builds.jabref.org/pull/11954/merge.

Merged via the queue into main with commit 2aa300a Oct 14, 2024
28 of 32 checks passed
@koppor koppor deleted the replace-buildinfo branch October 14, 2024 21:04
@github-actions GitHub Actions
Copy link
Contributor

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

@koppor koppor mentioned this pull request Oct 15, 2024
Merged
7 tasks
ExrosZ pushed a commit to ExrosZ/jabref that referenced this pull request Oct 17, 2024
@calixtus @ExrosZ-Alt
Replace new instantiation of BuildInfo with calls to Injector (JabRef…
1024dbe 
…#11954)

* Replace new instantiation of BuildInfo with calls to Injector

* Extract call to default keys out of fetchers

* Inject buildInfo

* Integrate SemanticScholarFetcher

* Integrate IEEE

* Add warning log

* Adapt devdocs

* Integrate SpringerLink

* Add comment

* Enhance doc

* Enhance doc
ExrosZ pushed a commit to ExrosZ/jabref that referenced this pull request Oct 17, 2024
@calixtus @ExrosZ-Alt
Replace new instantiation of BuildInfo with calls to Injector (JabRef…
82542cc 
…#11954)

* Replace new instantiation of BuildInfo with calls to Injector

* Extract call to default keys out of fetchers

* Inject buildInfo

* Integrate SemanticScholarFetcher

* Integrate IEEE

* Add warning log

* Adapt devdocs

* Integrate SpringerLink

* Add comment

* Enhance doc

* Enhance doc
ExrosZ pushed a commit to ExrosZ/jabref that referenced this pull request Oct 21, 2024
@calixtus @ExrosZ-Alt
Replace new instantiation of BuildInfo with calls to Injector (JabRef…
b60d898 
…#11954)

* Replace new instantiation of BuildInfo with calls to Injector

* Extract call to default keys out of fetchers

* Inject buildInfo

* Integrate SemanticScholarFetcher

* Integrate IEEE

* Add warning log

* Adapt devdocs

* Integrate SpringerLink

* Add comment

* Enhance doc

* Enhance doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koppor koppor koppor approved these changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: code-quality Issues related to code or architecture decisions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@calixtus @koppor @Siedlerchr @InAnYan