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

Better checks for Changelog entries #77

Merged
merged 15 commits into from
Feb 20, 2024
Merged

Conversation

neelasha23
Copy link
Contributor

@neelasha23 neelasha23 commented Feb 12, 2024

Closes #76

path

logs

logs

logs

logs

logs

logs

logs

logs

fixed cmd

fixed cmd

fixed cmd

fixed cmd

fixed cmd

fixed cmd

parse entry

git cmd

parser

parser

parser

parser

parser

restructured

restructured

restructured

restructured

test

Lint

tests

remove binder test

remove print

fix test
@neelasha23 neelasha23 marked this pull request as ready for review February 13, 2024 17:49
@neelasha23
Copy link
Contributor Author

Please review @edublancas

@edublancas
Copy link
Collaborator

@bryannho please dome some testing

you can open a dummy PR in this repo to test it: https://github.com/ploomber/dummy

just ensure you're installing pkgmt from the right place

pip install https://github.com/neelasha23/pkgmt@issue76

if you have questions about installation and testing let us know

@bryannho
Copy link
Contributor

bryannho commented Feb 14, 2024

@edublancas @neelasha23 Did some testing, you can see my tests here: ploomber/dummy#64

Tested these cases:

  • PASS: if the changelog in the PR modifies the contents of the 0.10.0dev section
  • FAIL: if there are changes in a section other than 0.10.0dev
  • FAIL: if there is no 0.10.0dev section
  • FAIL: if the 0.10.0dev has changed but the changes are only whitespace
  • FAIL: if a previous section is removed
  • FAIL: the 10.0.0dev section is replaced by 10.0.0 (removing the dev)
  • FAIL: if a previous entry in the 0.10.0dev section is removed

All cases work as expected except for the main one: If Changelog.md is not modified at all, but another file is, the test passes when it should fail. Meaning this catch isn't working for some reason. See this commit and the CI check. You can also look at the files changed to see I didn't modify the changelog. LMK if I'm testing incorrectly but I'm pretty sure this is an error since the other cases behaved as expected.

Another note is I had to modify ci.yml here, so to make these changes work in other repos we'll have to make the same change in each one.

@neelasha23
Copy link
Contributor Author

Fixed the bug that @bryannho mentioned. here's the commit and the check

Please review again

@bryannho @edublancas

@edublancas
Copy link
Collaborator

@bryannho don't forget to perform reviews, remember that we have a 24 hours SLA

Copy link
Contributor

@bryannho bryannho left a comment

Choose a reason for hiding this comment

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

@neelasha23 @edublancas tested, bug fixed. looks good to me!

@edublancas edublancas merged commit 1a8a0b1 into ploomber:main Feb 20, 2024
7 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.

improve check changelog feature
3 participants