refactor: opt-in for non-reproducible builder options #121
Conversation
drahnr
force-pushed
the
bernhard-builder-opt-in
branch
3 times, most recently
from
7476076
to
1e6f698
Compare
|
@drahnr Let's do a new release after we finish up + merge this PR and the payloaddigest one. |
drahnr
force-pushed
the
bernhard-builder-opt-in
branch
from
044c1e2
to
d18bd5c
Compare
| .add_changelog_entry("me", "was awesome, eh?", 123_123_123) | ||
| .add_changelog_entry("you", "yeah, it was", 12_312_312) | ||
| .add_changelog_entry( | ||
| "me", |
There was a problem hiding this comment.
We should use the standard packaging guidelines for the author value, which means
FirstName LastName <email> - VersionAndRelease
The description should start with -
So for the changelog entry (in a specfile)
%changelog
* Tue May 31 2016 Adam Miller <maxamillion@fedoraproject.org> - 0.1-1
- First bello package
- Example second
The date would go to date, "Adam Miller <maxamillion@fedoraproject.org> - 0.1-1" would go to "author", and
- First bello package
- example second item in the changelog for version-release 0.1-1
would be the description
There was a problem hiding this comment.
Well for the tests it doesn't matter much, but I mean for examples and such
There was a problem hiding this comment.
I think that needs some more refinement, why would we make the version suffix - 0.1-1 part of the author? It appears the API is not quite there and I'd like to carve that out in a separate PR.
|
@drahnr After that ^^ this is good to merge |
I don't necessarily want to change the API in this PR, which imho is necessary. I changed the author and added |
|
I don't think we need to change the API. It's weird, I agree, but the fact is that the data is actually stored together under the same IndexTag, and other libraries like createrepo_c keep them together in the API. What you could do is change |
We had the dependency already, debugging becomes easier and the codebase more unified.
Co-authored-by: Daniel Alley <dalley@redhat.com>
drahnr
force-pushed
the
bernhard-builder-opt-in
branch
from
ef8157a
to
c70b7ca
Compare
src/rpm/builder.rs
Outdated
| /// ``` | ||
| pub fn add_changelog_entry<A, E, TZ>( | ||
| mut self, | ||
| author: A, |
There was a problem hiding this comment.
e.g. you could put something along these lines (feel free to reword) in the doc comment
Convention is to have one changelog entry per release. The changelog name should be comprised of the first and last name of the release publisher, followed by their email address inside angle brackets, followed by a hyphen separator, followed by the version identifier of the release. Changes within the release should be listed in a bulleted list, one-per-line, using a hyphen character as a bullet.
There was a problem hiding this comment.
And rename the parameter
| author: A, | |
| changelogname: N, |
drahnr
force-pushed
the
bernhard-builder-opt-in
branch
from
c70b7ca
to
9de586f
Compare
|
The entire API here is just quirkesque. I'll create a separate PR to straighten this with a separate type that exposes a proper builder pattern. |
|
I don't think it's terribly quirky tbh, it's in line with what any other library does. |
It's not idiomatic Rust, it's not intuitive for anyone using it the first time and issues are only encountered with other libraries validating the changelog format. That is bad UX and should be avoided where possible. |
|
Failing test |
Fixed |
Makes sure
build_timeandbuild_hostare opt-in.Not clear, if there is a reason to keep
cookie. We're a lib, and as such that's more a design choice for the user of the library than us.Ref #117