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

DI-1 #687

Closed
wants to merge 32 commits into from
Closed

DI-1 #687

wants to merge 32 commits into from

Conversation

calixtus
Copy link
Collaborator

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 marked this pull request as draft May 23, 2024 04:48
Repository owner deleted a comment from github-actions bot May 23, 2024
Repository owner deleted a comment from github-actions bot May 23, 2024
Repository owner deleted a comment from github-actions bot May 23, 2024
Repository owner deleted a comment from github-actions bot May 23, 2024
Repository owner deleted a comment from github-actions bot May 23, 2024
Repository owner deleted a comment from github-actions bot May 23, 2024
Repository owner deleted a comment from github-actions bot May 23, 2024
@calixtus calixtus changed the title DI DI-1 May 23, 2024
@koppor
Copy link
Owner

koppor commented May 27, 2024

This is an alternative to use Google's Dagger , which was tried out 2.5 years ago at #518

@koppor koppor mentioned this pull request May 27, 2024
5 tasks
@koppor
Copy link
Owner

koppor commented May 27, 2024

We can use this PR to do a proper renaming:

Please fix the name "PreferenceService".

Reason:

@koppor
Copy link
Owner

koppor commented May 27, 2024

I think, this is the right place to provide the link to some naming hints and usages JabRef#6103 (comment)

# Conflicts:
#	src/main/java/org/jabref/gui/fieldeditors/OptionEditor.java
#	src/main/java/org/jabref/gui/util/component/TemporalAccessorPicker.java
@calixtus
Copy link
Collaborator Author

calixtus commented May 27, 2024

We can use this PR to do a proper renaming:

Please swap the names "PreferenceService" and "Preferences".

Can we postpone this to a follow up PR? I believe this one is on the edge of what can be understood in one PR.
Thats why I called the PR DI-1 😄
I just wanted to finally get rid of Globals and prepare the next steps by unifying the access and the injection method.

isDebugEnabled = false;
}

// addLogToDisk
Copy link
Owner

Choose a reason for hiding this comment

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

Just a comment no action needed:

Here, we see different coding styles. I used method names to avoid comments and to enable grouping of related statements. For you, it seemed that grouping is too fine grained and you wanted to group using lines. -- For me having the single line comment // addLogToDisk spanning statements separated by empty lines was unclear (also because there is the comment The "Shared File Writer" ....

@calixtus calixtus closed this May 27, 2024
@koppor
Copy link
Owner

koppor commented May 27, 2024

Follow-up at JabRef#11342

@koppor koppor deleted the di branch May 27, 2024 19:36
@calixtus calixtus mentioned this pull request May 27, 2024
6 tasks
@koppor
Copy link
Owner

koppor commented May 27, 2024

We can use this PR to do a proper renaming:
Please swap the names "PreferenceService" and "Preferences".

Can we postpone this to a follow up PR?

Please directly after merge as DI-2.

@koppor koppor mentioned this pull request May 29, 2024
6 tasks
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.

2 participants