-
Notifications
You must be signed in to change notification settings - Fork 24
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
483: support multiple editions in update #555
483: support multiple editions in update #555
Conversation
…date # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/tool/python/PythonUrlUpdater.java # cli/src/main/java/com/devonfw/tools/ide/url/updater/JsonUrlUpdater.java
Pull Request Test Coverage Report for Build 10628491050Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slskiba thanks for your PR. You nicely added the ability to pass the edition as parameter instead of internally retrieving it from the getter. The way you add multiple editions as List without the need to refactor all other UrlUpdaters is smart and well designed. 👍
The only little room for improvement that I can notice is that you are parsing the response as JSON for IntelliJ and Python multiple times only to get one item out of the JSON array. You could theoretically leave the getJsonObjectFromResponse
without the edition
and create a new parent container object for that and containing the editions. Then you could add a protected method getJsonEdition(J rootObject, String edition)
to JsonUrlUpdater
that gets the root JSON object and returns the actual edition JSON object from it for a given edition
parameter. The you however, need 3 generic types: J, JE, JVI
where JE
is the JSON edition object. Other JSON UrlUpdaters would then declare J = JE
. To avoid changes in other URL Updaters, you could extract AbstractJsonUrlUpdater
that has these 3 generics while JsonUrlUpdater
extends it and implement getJsonEdition
to just return rootObject
.
However, I do not see a problem with the current approach and we can also merge as is...
Closes #483