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

Improve documentation and add a few small features and fixes #210

Merged
merged 11 commits into from
Jan 9, 2024

Conversation

dralley
Copy link
Collaborator

@dralley dralley commented Jan 7, 2024

I'm trying to split the irrelevant stuff out of my other PRs and get it merged a bit sooner, while I'm working out a few details on the more complex stuff. Making those PRs smaller is also nice.

The reason for all the little features is that the gaps were encountered in the process of writing new tests.

I went through some effort to make sure the commits are split cleanly as opposed to the other PRs where they got mashed up a bit.

📜 Checklist

  • Commits are cleanly separated and have useful messages
  • A changelog entry or entries has been added to CHANGELOG.md
  • Documentation is thorough
  • Test coverage is excellent and passes
  • Works when tests are run --all-features enabled

@dralley dralley force-pushed the cleanups branch 3 times, most recently from 3668cf5 to ecef365 Compare January 7, 2024 20:46
const QFORMAT = 1 << 1;
/// Critical for success/failure,
///
/// Corresponds to RPMSCRIPT_FLAG_CRITICAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there is no rpm spec, I find it helpful to have references to the C code names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really want super direct references to the RPM source code if it can be helped

src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
src/rpm/headers/types.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few nits:

  • %foo should always be code, generally more inline code blocks would allow to use i.e. tools like cargo-spellcheck without too many false positives eventually
  • inconsistent TODO vs @todo lines, we shouldn't have any but track them in issues

other than that,

LGTM 👍

src/rpm/headers/types.rs Outdated Show resolved Hide resolved
@dralley dralley merged commit 440dc70 into rpm-rs:master Jan 9, 2024
15 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.

2 participants