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

(musescore) Cleanup update.ps1 links #2326

Conversation

umol-unibas
Copy link
Contributor

Description

The changed hard-coded link is not necessary in the old format and the way it is worded suggests that the download location is sufficient. There is already a valid download-link generated and pushed to the VERIFICATION.txt, so there is no need for a second one.

Motivation and Context

In the VERIFICATION.txt were two msi-links included. As the first one is not necessary and suggests that the download location is sufficient, it would be great if it could be changed, because of some automations we are using.

How Has this Been Tested?

This change does not harm or interrupt the update-module and its process at all as the link is only there for informational purpose.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Migrated package (a package has been migrated from another repository)

Checklist:

  • My code follows the code style of this repository.
  • My change requires a change to documentation (this usually means the notes in the description of a package).
  • I have updated the documentation accordingly (this usually means the notes in the description of a package).
  • I have updated the package description and it is less than 4000 characters.
  • All files are up to date with the latest Contributing Guidelines
  • The added/modified package passed install/uninstall in the chocolatey test environment.
  • The changes only affect a single package (not including meta package).

@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

Copy link

@stokes-unibas stokes-unibas left a comment

Choose a reason for hiding this comment

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

Looks good to me, tested and works

automatic/musescore/update.ps1 Outdated Show resolved Hide resolved
@AppVeyorBot
Copy link

✅ Package verification completed without issues. PR is now pending human review

1 similar comment
@AppVeyorBot
Copy link

✅ Package verification completed without issues. PR is now pending human review

@pauby
Copy link
Member

pauby commented Dec 21, 2023

This latest change is being made in the wrong place. If we are going to hard code the link we should just update the VERIFICATION.txt file directly and remove the updating of 'location on' that is done here. There isn't any point in automatically updating a file with the same text each time if it's not going to change. And if it does change then we need to update the update script anyway so it's no more hassle updating the VERIFICATION.txt file directly.

@umol-unibas Does that make sense?

@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

@umol-unibas
Copy link
Contributor Author

You are right @pauby. There is no need in updating the update.ps1 as it works fine - i'm not sure why i changed that. According to your message and our discussion i reverted the update-script and changed the VERIFICATION.txt. That was my idea was in the first place for this PR.

Reasons for the changes in the VERIFICATION.txt: the hard-coded link ("https://musescore.org/en/download/musescore.msi ") is

  • not necessary as it is a duplication
  • it could be puzzling because of the other link
  • it has no meaning

the link isn't necessary in the old format and the way it is worded suggests that the download location is sufficient
@AppVeyorBot
Copy link

❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look.

@AppVeyorBot
Copy link

✅ Package verification completed without issues. PR is now pending human review

Copy link
Member

@AdmiringWorm AdmiringWorm left a comment

Choose a reason for hiding this comment

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

LGTM

@AdmiringWorm AdmiringWorm merged commit 0c36a69 into chocolatey-community:master Jan 2, 2024
1 check passed
@umol-unibas
Copy link
Contributor Author

Thanks!

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.

5 participants