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

#437 Refactor npmurlupdater #445

Closed

Conversation

slskiba
Copy link
Contributor

@slskiba slskiba commented Jul 5, 2024

Fixes #437.

  • Implements NpmBasedUrlUpdater as an extension of JsonUrlUpdater
  • NpmUrlUpdater is now a NpmBasedUrlUpdater instead of a WebsiteUrlUpdater
  • Added Tests for NpmUrlUpdater

There is a rather unclean workaround to fix versions being filtered properly for npm, so that alpha/beta versions are not being made available. These versions were not available with the prior used WebsiteURLUpdater. This problematic behaviour however is not specific to npm: Currently, e.g. python too makes alpha, beta and rc versions available to the user.
This problem arises due to these tools implementing their own update method, in which doAddVersion instead of addVersion of the AbstractUrlUpdater is being utilized, and by that, circumventing the mapVersion call in which these versions would have been filtered.

I am open to suggestions for a prettier solution for this workaround, however it seems to me it requires some refactoring on AbstractUrlUpdater, and might be best to do this in a seperate issue, and consider the refactoring of NpmUrlUpdater completed.

@slskiba slskiba self-assigned this Jul 5, 2024
@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9805805158

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.09%) to 61.126%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/tool/npm/NpmUrlUpdater.java 1 66.67%
Totals Coverage Status
Change from base Build 9800169993: 0.09%
Covered Lines: 5207
Relevant Lines: 8193

💛 - Coveralls

@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9806511852

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 61.158%

Totals Coverage Status
Change from base Build 9800169993: 0.1%
Covered Lines: 5207
Relevant Lines: 8189

💛 - Coveralls

@slskiba slskiba marked this pull request as ready for review July 5, 2024 10:32
@slskiba slskiba changed the title #437 WIP: Refactor npmurlupdater #437 Refactor npmurlupdater Jul 5, 2024
Comment on lines +58 to +62
//TODO: this is not the right place to filter versions
// Also missing the logging of AbstractUrlUpdater's addVersion on which versions were filtered
if (mapVersion(version) == null) {
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the workaround mentioned above.

@coveralls
Copy link
Collaborator

coveralls commented Jul 5, 2024

Pull Request Test Coverage Report for Build 9807023953

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.5%) to 61.509%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java 3 90.11%
Totals Coverage Status
Change from base Build 9800169993: 0.5%
Covered Lines: 5253
Relevant Lines: 8224

💛 - Coveralls

@slskiba slskiba requested a review from jan-vcapgemini July 8, 2024 08:44
@slskiba slskiba marked this pull request as draft July 9, 2024 07:34
@slskiba
Copy link
Contributor Author

slskiba commented Jul 9, 2024

Needs another refactoring round, moving away from overriding update, but instead implement addVersion and collectVersionsFromJson. This will also be necessary for the UrlUpdaters of AndroidStudio and Python, for which a separate Issue #458 has been opened.
Since NpmUrlUpdater needs to be reimplemented differently from the approach taken in this PR I will close this PR and open a new PR once NpmUrlUpdater is the approach discussed above.

@jan-vcapgemini jan-vcapgemini mentioned this pull request Jul 9, 2024
5 tasks
@slskiba slskiba closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Refactor NpmUrlUpdater
2 participants