Add lint rule for "~ppa" in version strings#4
Conversation
Passed in by dput when the hooks are called.
enr0n
left a comment
There was a problem hiding this comment.
Thanks! I think this is a good addition overall, just some requests on the implementation.
| def __init__( | ||
| self, | ||
| changes: str | deb822.Changes | None = None, | ||
| profile: dict | None = None, |
There was a problem hiding this comment.
Hm, I am not sure it makes sense to add this to Context. It's mean to wrap generic things about the package itself. But this check is really a pure dput hook; it doesn't make sense to run this check except when uploading, because it depends on the upload target.
It would be different than the other checks so far, but I think it might make more sense to write this as a pure hook in dput.py.
There was a problem hiding this comment.
I agree it doesn't feel great to add this to context for essentially a single hook, but adding this as a pure hook in dput.py also breaks with the existing architecture of this repo.
I would argue enlarging the context object makes more sense than essentially bypassing half your work by adding a pure hook, since the upload target is context and arguably could be used by other hooks, but I'll leave it up to you which way you prefer the implementation.
There was a problem hiding this comment.
Let's go with the pure dput hook for now. It doesn't really feel like it's bypassing anything substantial to me.
Also, the dput hooks may ultimately be upstreamed to dput-ng anyways. That's part of why they are separated like this
Checks that "~ppa" is not present in uploads to the archive, or that it is present for uploads to a PPA. Also added a test for the new linter, including mock profiles to use when testing. Added a new profile for ppa uploads, which only contains the new hook for now.
dd2849c to
f2eda5b
Compare
Added a check requiring "~ppa" in the version string for PPA uploads and the opposite for archive uploads. Includes a change to the
Contextobject required to pass in the upload profile to detect upload destination, as well as a test for the new linter.Also added a new
ppaprofile which runs when an upload is meant for a PPA. Currently only includes the newly added check.Open to feedback on the hook name, as well as whether or not it should count as a fail for PPA uploads instead of a warn (I think why not, though I may have missed valid contexts for doing so).