-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
remove jython #7157
Conversation
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 |
I guess it can be removed. (I have not used the script since we moved the localization to CrownIn). |
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. |
@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? |
@koppor The l10n is handled by crowdin, so I think this is no longer necessary or? |
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; |
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). |
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. |
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 |
Yes, that's exactly what the purpose was. It's nowhere used in our workflows. |
was just a test
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. |
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. |
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 😅 |
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) |
The code how to already contains a section with the exact steps. |
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. |
* 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)
I stumbled across the jython dependency and wondered if its still necessary