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

Ergonomics and docs #5

Merged
merged 6 commits into from
Nov 26, 2023
Merged

Ergonomics and docs #5

merged 6 commits into from
Nov 26, 2023

Conversation

fbenkstein
Copy link
Collaborator

This PR tries to attempt a couple of issues at once:

  • Add a SquidBuilder struct as an alternative to Options. I believe this
    makes the code more readable. For example instead of
    let sqids = Sqids::new(Some(Options::new(
      None,
      Some(10),
      None,
    )))?;
    
    We can now write:
    let sqids = Sqids::builder()
      .min_length(10)
      .build()?;
    
    If this is accepted I suggest deprecating Options in favor of
    SqidsBuilder and removing it in the 1.0 release.
  • Pull in the README.md file as a crate-level doc-comment. This makes the
    docs much more approachable since now the main doc page has actual useful
    information instead of being a barren waste land. Since the README.md
    links to the LICENSE file I used a small hack to ensure that this link is
    still valid.
  • Pulling in the README.md file automatically turned the code examples into
    documentation tests. These were immediately broken because of missing imports
    and error handling. I've added the necessary boilerplate. It's hidden by
    rustdoc but unfortunately looks a bit ugly when viewing the README.md on
    Github.
  • Add basic doc comments to every public item and warn when new items are added
    without docs. If I were maintainer of this crate, I'd turn this into a
    deny.

Let me know if you want me to pull this apart into multiple PRs instead.

Include the README.md file as top-level doc item in lib.rs. This
automatically makes the Examples into doc-tests so they have to be
adjusted slightly. Also, introduce a dummy LICENSE constant since that
is linked from the README.md.
@4kimov 4kimov merged commit 6df6b7a into sqids:main Nov 26, 2023
1 check passed
@4kimov
Copy link
Member

4kimov commented Nov 26, 2023

@fbenkstein This PR is pure 🔥 -- thank you. Merged 💪

I've sent you an invite with maintainer rights to this repo, so you can push without being blocked by me.

Some follow-ups:

  • The builder pattern is great, I've been eyeing it for a while. I don't mind removing the Options even in the next release (0.4.0); we're going to be pre-production for a while since we're testing not just the individual implementations, but the algo/spec as well.

  • All the readme/docs updates are great 👍

  • If I were maintainer of this crate, I'd turn this into a deny

    That's cool with me.

@fbenkstein
Copy link
Collaborator Author

@4kimov thanks for the quick turnaround.

I've sent you an invite with maintainer rights to this repo, so you can push
without being blocked by me.

Thank you, accepted. I'll send a PR anyway unless it's something really tiny.

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