Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Changelog to auto skip when no code change of interest detected #100

Closed
wants to merge 3 commits into from

Conversation

pllim
Copy link
Member

@pllim pllim commented Dec 23, 2018

This address OpenAstronomy/baldrick#64 but in ways that is specific to astropy core repo.

🎉 100 🎉

@codecov
Copy link

codecov bot commented Dec 23, 2018

Codecov Report

Merging #100 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #100     +/-   ##
=========================================
+ Coverage   98.61%   98.81%   +0.2%     
=========================================
  Files           3        3             
  Lines         144      169     +25     
=========================================
+ Hits          142      167     +25     
  Misses          2        2
Impacted Files Coverage Δ
astropy_bot/tests/test_changelog_checker.py 100% <100%> (ø) ⬆️
astropy_bot/changelog_checker.py 95.91% <100%> (+0.91%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b185bc7...65aec4a. Read the comment docs.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

In principle this looks good but I think we should make it a little less core-package specific

('Affects-dev' not in pr_handler.labels)):
statuses['changelog'] = {'description': 'Changelog not required for these changes',
'state': 'success'}
return statuses
Copy link
Member

Choose a reason for hiding this comment

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

Some of this logic is a bit too specific to the core package (the bot can be used on any other packages). It's also not 100% clear to me that files outside of astropy and cextern never require a changelog entry (e.g. setup.cfg). I'd suggest therefore just limiting has_code_change to just checking that the files aren't all tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, now I am confused. I thought core-specific stuff goes here and generic stuff goes to baldrick? Where is core-specific stuff supposed to go, then?

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, the stuff here is specific to the astropy project, not just the core package - so for example we use it for astropy-helpers too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh... 🤔 Any idea where to put core specific stuff? I was gonna expand it to auto add subpackage labels next and that is very core specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then what is baldrick for? SunPy is part of the Astropy project too, right, since it is affiliated? (I am just confused, not disagreeing.)

Copy link
Member Author

@pllim pllim Jan 9, 2019

Choose a reason for hiding this comment

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

I will have to think about this... Making this too generic might also render it pretty much useless. One of the original request was to auto add the label if only doc change is detected. To detect doc change in a more generic way is more complicated, so that is why I did it this way for shorter if checks. 🤔 🤔 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@astrofrog , better now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants