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

Add API to write all items into buffer #157

Closed
SUPERCILEX opened this issue Sep 21, 2022 · 10 comments
Closed

Add API to write all items into buffer #157

SUPERCILEX opened this issue Sep 21, 2022 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@SUPERCILEX
Copy link
Contributor

I have an expect test that checks for public API changes, but I currently have to dig into internals to get it working: https://github.com/SUPERCILEX/ftzz/blob/bc6779dd0b213f3c2c1150fb6ae8913674240815/tests/api.rs. It'd be nice to have an API that takes in a Write and spits out the items.

@Enselic
Copy link
Member

Enselic commented Sep 21, 2022

Thank you for the feedback! Looks to me like you are using the API just as intended; you are not using internals. In fact, the code you use is the same as the example code at the top-level docs: https://docs.rs/public-api/latest/public_api/

I'm not sure it makes sense to have an even higher level wrapper, but I don't want to shoot down this idea before having seen a PR with a concrete proposal :)

@Enselic Enselic added the enhancement New feature or request label Sep 21, 2022
@Emilgardis
Copy link
Member

I think what could be done is moving cargo_public_api::Plain to public_api::Plain and placing the item formatter behind a impl fn

should be pretty straight forward

@Emilgardis Emilgardis self-assigned this Sep 21, 2022
@Enselic
Copy link
Member

Enselic commented Sep 21, 2022

@Emilgardis That would make public-api crate depend on terminal color crates, and I'd rather not have the public API analyzer depend on colors. I also try to avoid Cargo features (i.e. I don't want it an optional dependency), because I think that complicates code/testing/development/maintenance too much. So the current setup is quite deliberate.

But maybe there is some aspect of this I am overlooking, so if you want to prototype a bit around it, feel free of course!

@Emilgardis
Copy link
Member

No it wouldn't as all the formatting would be controlled by the caller, it's possible to implement without any new dependencies!

@Enselic
Copy link
Member

Enselic commented Sep 21, 2022

Sounds like you have thought this through, please go ahead, looking forward to the PR! 😀

@SUPERCILEX
Copy link
Contributor Author

Oh I had no idea that was the intended use case :). I'm fine with the issue being closed though what I was basically thinking of doing is to make this method public and have it take in the Write as a param: https://github.com/Enselic/cargo-public-api/blob/6c84ec97a2c4d7e8137bd872bf6fe39906e3855f/public-api/src/main.rs#L57-L65

@Emilgardis
Copy link
Member

yup, my initial comment is a bit unneccessary parsing through what is actually needed now, a convenience function like this makes sense.

@Enselic
Copy link
Member

Enselic commented Sep 21, 2022

I'm actually not sure it makes sense to put that helper in the public API, because in the near future, we might want to make the output more tree-like, to solve #36, #37 and #39. The larger the API surface we have, the more work it will be to make necessary adaptations to the API. I try to keep the API surface minimal, because any addition to it, however small, increases maintenance burden of this project, and spare time is a scarce resource. Keep in mind that we not only have to maintain the API surface itself, but also tests for the API surface.

(Sorry for spreading some negativity again)

I'm fine with the issue being closed

If that is the case, and if @Emilgardis is also OK with that, I think we should do that.

Thanks again @SUPERCILEX for bringing this up so we could discuss it though.

@Emilgardis
Copy link
Member

I'll implement what I have in mind and we can see if it makes sense, to me it doesn't bother me if it is not merged :)

@Emilgardis
Copy link
Member

Aaaand thinking about this a bit more, I think this is more a documentation issue :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants