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

remove jython #7157

Merged
merged 8 commits into from
Dec 5, 2020
Merged

remove jython #7157

merged 8 commits into from
Dec 5, 2020

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Dec 3, 2020

I stumbled across the jython dependency and wondered if its still necessary

  • Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@tobiasdiez
Copy link
Member

This readme has to be removed as well: https://github.com/JabRef/jabref/blob/8777c16a99bd83e4a30ddafc859c0c2208b68733/scripts/README.md

I'm not sure if the script is still needed somewhere, maybe for the documentation? @mlep @koppor
In either case, I think, it is ok to remove the gradle integration and simply keep the python script.

@tobiasdiez tobiasdiez added status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Dec 4, 2020
@mlep
Copy link
Contributor

mlep commented Dec 4, 2020

I guess it can be removed. (I have not used the script since we moved the localization to CrownIn).

@koppor
Copy link
Member

koppor commented Dec 4, 2020

Ah, now I remember. I always used the scripts directly. Others wanted to execute it directly in IntelliJ. The argument was consistency in task execution: Have all development tasks available in the Gradle task list.

OK for me to remove Jython.

We need to rewrite our GitHub workflow IMHO. If a contributor adds a new language string, the script checks whether it is also available in the language property file. Thus, we need to execute the script and let the workflow fail if that happens.

@koppor
Copy link
Member

koppor commented Dec 4, 2020

@Siedlerchr Please add some motivation text to the PR text. Is it because of the jlink issue or just because of the size of build.gradle?

@Siedlerchr
Copy link
Member Author

@koppor The l10n is handled by crowdin, so I think this is no longer necessary or?

@koppor
Copy link
Member

koppor commented Dec 4, 2020

Crowdin DOES NOT parse the .java files. Crowdin handles *. properties only

Just try to add Localization.lang("JabRef super powers"); to any .java file and see whether this PR fails. Then do it in another branch an create a pull request and see what happens.

You can make screenshots then and add it to our development documentation.

I really need a check whether Crowdin knows all strings used in *.java. moreover, deleted strings should also be removed in *.properties.

In short: .java - en.properties: script;
en - non-en: crowdin

@koppor
Copy link
Member

koppor commented Dec 4, 2020

Please do not delete the script.

Please change the documentantation at https://devdocs.jabref.org/getting-into-the-code/code-howtos#using-localization-correctly

Installation of Python needs to be added. I am not sure whether we can ask all developers to have a properyl configured Pyhton environment to check for the localizations.

I really need to understand the motivation of this PR (since I should work on other JabRef issues).

@koppor
Copy link
Member

koppor commented Dec 4, 2020

Maybe we should discuss it in the devcall.

However, I do not want to start a discussion again whether Localization is necessary at all and why we are doing it as is. I think, we have other issues to tackle to get a release done.

@tobiasdiez
Copy link
Member

The script is not used in any of github workflows (as far as I see, and shown by the passing checks). The unittests that we have to check that all lang(...) strings are in the properties file are written in java. The python script apparently was used to manually sync the language files, which is indeed no longer necessary.

@Siedlerchr
Copy link
Member Author

The script is not used in any of github workflows (as far as I see, and shown by the passing checks). The unittests that we have to check that all lang(...) strings are in the properties file are written in java. The python script apparently was used to manually sync the language files, which is indeed no longer necessary.

Yes, that's exactly what the purpose was. It's nowhere used in our workflows.

was just a test
@koppor
Copy link
Member

koppor commented Dec 4, 2020

And we add the missing strings manually to the properties file or do we have junit "test" for that?

The python script was able to that. I agree that we should have that functionality in Java, too.

@Siedlerchr
Copy link
Member Author

And we add the missing strings manually to the properties file or do we have junit "test" for that?

If you add a new string in the java files somewhere, you need to add it to the en.properties. Crowdin does the sync between different languages. This has always been the case.

@koppor
Copy link
Member

koppor commented Dec 4, 2020

I totally forgot about the copy'n'pastable output 😅.

We did not add an ADR back then stating that we accept the manual work at that place gaining a step towards deletion a python script. We want Java only in this project 😅

@koppor
Copy link
Member

koppor commented Dec 4, 2020

Since we are in the context: Can the code how-to be extended with the dealing of new language strings? (Maybe I can do it tomorrow)

@Siedlerchr
Copy link
Member Author

The code how to already contains a section with the exact steps.

@koppor
Copy link
Member

koppor commented Dec 4, 2020

I thought about the GitHub checks. Because we have one here. Adding that would help newcomers getting in touch with GitHub actions about the first times. Having a real example would be handy IMHO.

@koppor koppor merged commit 161e928 into master Dec 5, 2020
@koppor koppor deleted the removejython branch December 5, 2020 22:45
Siedlerchr added a commit that referenced this pull request Dec 6, 2020
* upstream/master:
  Improve library loading UX (#7119)
  remove jython (#7157)
  Add missing authors
  Remove obsolete hint
  Fixed issue in PreviewView for number textfield (#7158)
  Fix for issue 6959 (#7151)
  Revert "remove jython (#7155)" (#7156)
  remove jython (#7155)
  Fix remembering password for sql db (#7154)
  Update to libre office 7.0.3 (#7150)
  Add IdBasedSearchFetcher to jstor (#7145)
  Squashed 'src/main/resources/csl-styles/' changes from 55200d0..a20406d
  Bump antlr4-runtime from 4.8-1 to 4.9 (#7136)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete 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.

4 participants