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

fix: dynamic grep latest MX linux and sort Kolibri #1479

Merged

Conversation

TuxVinyards
Copy link
Contributor

Description

Fixes:

FAIL: mxlinux-23.3-Xfce https://sourceforge.net/projects/mx-linux/files/Final/Xfce/MX-23.3_x64.iso

FAIL: mxlinux-23.3-KDE https://sourceforge.net/projects/mx-linux/files/Final/KDE/MX-23.3_KDE_x64.iso

FAIL: mxlinux-23.3-Fluxbox https://sourceforge.net/projects/mx-linux/files/Final/Fluxbox/MX-23.3_fluxbox_x64.iso

Type of change

Hopefully 'maintenance free'

  • [ x] Bug fix (non-breaking change which fixes an issue)

Checklist:

  • [ x] I have performed a self-review of my code

  • [ x] I have tested my code in common scenarios and confirmed there are no regressions

  • [ x] I have added comments to my code, particularly in hard-to-understand sections

@TuxVinyards TuxVinyards changed the title Fix: add dynamic grep for latest MX linux releases fix: add dynamic grep for latest MX linux releases Oct 15, 2024
@TuxVinyards TuxVinyards changed the title fix: add dynamic grep for latest MX linux releases fix: Add dynamic grep for latest MX linux releases Oct 15, 2024
Also as  requested by code validation for github actions

Amend the commit message to match the pull request title, or *add another commit.*
tested, loads  and runs ...
make code style more consistent

move basename line into better position
Copy link
Contributor

@philclifford philclifford left a comment

Choose a reason for hiding this comment

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

There are commits mixed in that make changes to KolibriOS. Please refactor these into a seperate PR for KolibriOS changes and limit this one to just the MX changes referred to in the title.
And thanks for catching and fixing up KolibriOS btw !

quickget Outdated Show resolved Hide resolved
@TuxVinyards TuxVinyards changed the title fix: Add dynamic grep for latest MX linux releases fix: dynamic grep latest MX linux and sort Kolibri Oct 17, 2024
@philclifford
Copy link
Contributor

See https://www.conventionalcommits.org/en/v1.0.0/
The merge checker's nagging is just to enforce "a single commit message must match the PR title exactly OR there must be several commits to merge". The PR should contain atomic commits that achieve the intent of the PR - ideally a single conventional commit.
It is preferable to keep the scope of the PR narrow - a single fix or feature. This way the resulting changelog is clear and straighforward and also the scope of changes that may need to be reviewed, tested and possibly ultimately rolled back or re-visited is kept small and coherent.

quickget Outdated Show resolved Hide resolved
hoping that spending time on this space will help fix the continuum ...
Header files are not always consistent in their format.
'Location' can sometimes be capitalized, hence the '-i'

Regex can too easily turn into lines of indecipherable hieroglyphics  but in this case, due to variance as cited above, regex would probably be more robust than grepping for 'location:' (with a colon)  and if the header was reorganised then '-m 1' could end up up failing.
On further research and reflection to 503e083

There are two types of location headers 'Location:' and 'Content-Location:'   so using the regex caret anchor ^'location'   could fail when direct links are used as it would only look for 'location' at the start of the line.

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers

" An HTTP header consists of its case-insensitive name followed by a colon (:), then by its value."

There is also the issue of different versions of grep having different regex capabilities to consider.

As such, I am now viewing a grep with  -i  'location:'  (with a colon)  as the more reliable general solution and suggest that this is the one to go with.
@philclifford philclifford merged commit 9d4b82d into quickemu-project:master Oct 19, 2024
5 checks passed
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.

2 participants