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

refactor: ex-export the Guess type #2

Merged
merged 1 commit into from
Sep 22, 2023
Merged

refactor: ex-export the Guess type #2

merged 1 commit into from
Sep 22, 2023

Conversation

SteveLauC
Copy link
Contributor

What does this PR do

  1. The Guess type defined in src/go/guess.rs is used by many public interfaces, but this type itself has not been exported so those interfaces that rely on this type are totally unusable, this PR fixes this.
  2. Add some doc comments for the public interfaces, the documents are copied from https://pkg.go.dev/github.com/go-enry/go-enry/v2#pkg-functions

@SteveLauC
Copy link
Contributor Author

Friendly ping @bzz, PTAL:)

Copy link
Member

@bzz bzz left a comment

Choose a reason for hiding this comment

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

Thank you very much for the documentation!

Overall, looks great, sans one question below.

use crate::go::guess::{GoGuess, Guess};
use crate::go::guess::GoGuess;

pub use go::guess::Guess;
Copy link
Member

@bzz bzz Sep 17, 2023

Choose a reason for hiding this comment

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

I'm no Rust expert and am curious why re-exporting Guess is required in there, i.e. what does this mean

interfaces that rely on this type are totally unusable

? (e.g. did that result in a warning/complains by the compiler or some other tooling, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your doubt is right, my statement is not technically correct, sorry for being inaccurate here.

The Guess type is indeed exposed and thus public, but its parent module src/go/guess.rs is not (pub(crate) mod guess, it is only visible within the crate) so those interfaces relying on it are usable, but users are not allowed to explicitly access it.

Since the Guess type is public, there are no compile errors or warnings.


However, it is still recommended to re-export the Guess type as it is used by public interfaces

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation. I'm a bit reluctant to extend public API surface without clear understand what specifically does the lack of re-export preclude the clients from.

But thinking more about it - although we don't have it in Go impl, we do seem to have Guess as part of API at e.g. Java bindings so it probably should be fine.

@SteveLauC SteveLauC changed the title fix: expose the Guess type refactor: ex-export the Guess type Sep 18, 2023
@SteveLauC
Copy link
Contributor Author

PR title and commit message updated: refactor: re-export the Guess type :)

@SteveLauC
Copy link
Contributor Author

Is there anything else I can do to improve this PR and finally get it merged?

@bzz bzz merged commit 3fd51cf into go-enry:main Sep 22, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants