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

Warn invalid files #1365

Merged
merged 2 commits into from
Sep 4, 2023
Merged

Warn invalid files #1365

merged 2 commits into from
Sep 4, 2023

Conversation

rzvxa
Copy link
Member

@rzvxa rzvxa commented Sep 2, 2023

Description of Changes

Closes #1274


New Version Info

Fixed issues with the original PR

Author's Instructions

  • Derive a new MAJOR.MINOR.PATCH version number. Increment the:
    • MAJOR version when you make incompatible API changes
    • MINOR version when you add functionality in a backwards-compatible manner
    • PATCH version when you make backwards-compatible bug fixes
  • Update CHANGELOG.md, following the established pattern.

Collaborator's Instructions

  • Review CHANGELOG.md, suggesting a different version number if necessary.
  • After merging, tag the commit using these (Mac-compatible) bash commands:
    git checkout master
    git pull
    sed -n "$(grep -n -m2 '####' CHANGELOG.md | cut -f1 -d: | sed 'N;s/\n/,/')p" CHANGELOG.md | sed '$d'
    git tag -a $(read -p "Tag Name: " tag;echo $tag) -m"$(git show --quiet --pretty=%s)";git push origin --tags

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Thanks for helping out with existing PRs. I would like to make sure we keep credit for the original author though. Somehow you've rewritten the original commits instead of keeping them. Can you please rebase this on the other PR commits so the original author meta data is kept, and then your commit appended to the list? At most I would squash them and use Co-authored-by headers, but separate commits is fine too. They just need to be at least partial credit to the OP.

@rzvxa
Copy link
Member Author

rzvxa commented Sep 3, 2023

Thanks for helping out with existing PRs. I would like to make sure we keep credit for the original author though. Somehow you've rewritten the original commits instead of keeping them. Can you please rebase this on the other PR commits so the original author meta data is kept, and then your commit appended to the list? At most I would squash them and use Co-authored-by headers, but separate commits is fine too. They just need to be at least partial credit to the OP.

Initially, I tried to rebase but Git complained about broken references as there were some removed commits here and there, So I just used cherry-picking to patch my fork, I can go back and get the original commit in, But it would be much easier to add credit on the merge commit.
If you don't want to do it this way, Tell me so I can go ahead and rebase it for real this time

@rzvxa rzvxa requested a review from alerque September 3, 2023 04:35
Rafael Monico and others added 2 commits September 5, 2023 00:56
@alerque
Copy link
Member

alerque commented Sep 4, 2023

Initially, I tried to rebase but Git complained about broken references as there were some removed commits here and there, So I just used cherry-picking to patch my fork, I can go back and get the original commit in, But it would be much easier to add credit on the merge commit.

No, there are no removed commits between that PR and this one. The are only 4 commits apart in the tree, and it would be impossible for the other PR to even be valid if it wasn't based on this repository. I didn't have any trouble rebasing your commits here onto the original PR.

If you don't want to do it this way, Tell me so I can go ahead and rebase it for real this time

I just did, and also merged one of the commits. You can give it a look here and if all looks okay to you I think we can merge.

@rzvxa
Copy link
Member Author

rzvxa commented Sep 4, 2023

@alerque Thanks for merging the initial PR, Looks good to me, Let's go ahead and merge this.

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