Skip to content
This repository was archived by the owner on Dec 1, 2022. It is now read-only.

Allow passing arguments to the rustdoc command #2

Merged
merged 4 commits into from
Apr 25, 2018
Merged

Allow passing arguments to the rustdoc command #2

merged 4 commits into from
Apr 25, 2018

Conversation

althonos
Copy link
Contributor

Hi there,

I was working on porting the pyo3 documentation to use docmatic (PyO3/pyo3#142), but unfortunately it does not work out of the box since it links to the libpython, which may be in a non-standard location.

I added the possibility to pass additional arguments to the rustdoc command, so that we can in our case pass additional --library-path flags.

The only drawback is that all arguments must have the same type, but it's safe to use String everywhere in that case.

(I also removed the glob dependency since it's not used anywhere...)

@althonos althonos changed the title Allow Allow passing arguments to the rustdoc command Apr 25, 2018
extern crate docmatic;

#[test]
fn test_readme() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, running the lib.rs docs as tests verifies the same behavior as these tests

src/lib.rs Outdated
.arg("--test")
.arg(documentation);

for arg in args.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

My thought for how to handle this case was to provide an API to wrap rustdoc rather than having people directly call rustdoc.

For example, we might have a builder that looks like

docmatic::Assert::new("README.md")
    .library_path(path)
    .unwrap()

or

docmatic::Assert::new()
    .library_path(path)
    .file("README.md")

Copy link
Contributor

Choose a reason for hiding this comment

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

I will say, if this seems like too much work at the moment, I can understand us leaving it as-is for now and then I can later go back in and refactor it.

Another option is we could mark the with variant as unsafe or hidden behind an unstable feature.

@epage
Copy link
Contributor

epage commented Apr 25, 2018

My preference on commits. This will keep the history cleaner and make it easier for me to identify things to put in the changelog.

With that said, I'm also understanding of people's different levels of experience with git and that some history clean up is enough work that it isn't worth doing (like moving a clippy fix across a major refactoring).

feat: add proper tests

Since this isn't a user-facing feature, I'd prefer it not to be feat. You could use test: Add initial tests or something

fix(crate): fix clippy linting issues

Since this also isn't user-facing, I'd put this down as a "chore" or "style". Or you could squash it.

fix(docs): add assert_file_with example to the module documentation

For this you could

  • Use fix
  • Use docs
  • Squash it into the original commit

feat: use generic types for arguments vector

Personally, I'd just squash this

fix(crate): use single-type slice for argument

This is another that I'd personally squash

@epage
Copy link
Contributor

epage commented Apr 25, 2018

I think I worked around the CI failure. Its an old problem that skeptic would also have but I finally have a better workaround than cargo clean.

So on next push the build "should" work.

@epage
Copy link
Contributor

epage commented Apr 25, 2018

btw thank you for contributing this and wanting to use docmatic! I hope this ends up working out for you.

@althonos
Copy link
Contributor Author

No problem ! I see what you mean, it would indeed be nicer to have a higher-level API for this. I'll propose another implementation with a builder pattern.

@epage epage merged commit 05978d6 into assert-rs:master Apr 25, 2018
@epage
Copy link
Contributor

epage commented Apr 25, 2018

Thanks!

I assume you want me to get a release out?

@althonos
Copy link
Contributor Author

The latest pyo3 build passed with the new API, so yes ! 😉

@epage
Copy link
Contributor

epage commented Apr 25, 2018

It is now published.

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

Successfully merging this pull request may close these issues.

2 participants