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

Make NuidWriter public #618

Merged
merged 7 commits into from
Sep 4, 2024
Merged

Make NuidWriter public #618

merged 7 commits into from
Sep 4, 2024

Conversation

Madgvox
Copy link
Contributor

@Madgvox Madgvox commented Aug 25, 2024

This PR makes the NuidWriter class public, and moves it into the Core namespace out of the Internal namespace.

Resolves #617.

@mtmk
Copy link
Collaborator

mtmk commented Aug 25, 2024

Thanks @Madgvox. Can I ask you to sign your commit please? It's a CNCF requirement, and we won't be able to merge because of GitHub rules setup for this repo. You will have to force push, which should be fine. Thank you 💯

@Madgvox Madgvox force-pushed the nuidwriter-public branch 2 times, most recently from 9288c68 to b01865f Compare August 25, 2024 09:01
@Madgvox Madgvox force-pushed the nuidwriter-public branch from b01865f to 019105b Compare August 25, 2024 09:10
@Madgvox
Copy link
Contributor Author

Madgvox commented Aug 25, 2024

Thanks @Madgvox. Can I ask you to sign your commit please? It's a CNCF requirement, and we won't be able to merge because of GitHub rules setup for this repo. You will have to force push, which should be fine. Thank you 💯

Should be corrected now.

@jasper-d
Copy link
Contributor

I think it would be better change the naming as follows:

- public static class NuidWriter {
+ public static class Nuid { // or NuidGenerator if you prefer
-   public static bool TryWriteNuid(Span<char> nuidBuffer);
+   public static int Write(Span<char> destination); // Throw for inappropriate sized destination, return number of chars written
}

Reasoning:

  1. Having GetString() on a type that is call "writer" seems a bit odd, calling the type Nuid or NuidGenerator avoids that issue.
  2. TryWriteNuid does not provide information how many chars are written, this is internal knowledge of the implementation.
  3. NuidWriter.TryWriteNuid duplicates notions of "nuid" and "write" unnecessarily
  4. I would expect callers to usually know how large their buffer is, so the try-pattern seems a bit unecessary here.
  5. If you want to keep the try-pattern (may possibly provide some perf-benefits with enough inlining and a suitable call-site), I'd propose bool TryWrite(Span<char> destination, out int charsWritten) to align better with other try-APIs in the BCL.
  6. Additionally, you may want to expose NuidLength as public static readonly int Length or something, so callers can use it when allocating buffers instead of relying on undocumented implementation details.

@mtmk
Copy link
Collaborator

mtmk commented Aug 27, 2024

I think it would be better change the naming as follows:

- public static class NuidWriter {
+ public static class Nuid { // or NuidGenerator if you prefer
-   public static bool TryWriteNuid(Span<char> nuidBuffer);
+   public static int Write(Span<char> destination); // Throw for inappropriate sized destination, return number of chars written
}

Reasoning:

  1. Having GetString() on a type that is call "writer" seems a bit odd, calling the type Nuid or NuidGenerator avoids that issue.

I'd be happy with either name

  1. TryWriteNuid does not provide information how many chars are written, this is internal knowledge of the implementation.
  2. NuidWriter.TryWriteNuid duplicates notions of "nuid" and "write" unnecessarily
  3. I would expect callers to usually know how large their buffer is, so the try-pattern seems a bit unecessary here.
  4. If you want to keep the try-pattern (may possibly provide some perf-benefits with enough inlining and a suitable call-site), I'd propose bool TryWrite(Span<char> destination, out int charsWritten) to align better with other try-APIs in the BCL.

This makes a lot of sense if we want to expose it. @Madgvox would you be using TryWrite?

  1. Additionally, you may want to expose NuidLength as public static readonly int Length or something, so callers can use it when allocating buffers instead of relying on undocumented implementation details.

If we want TryWrite it'd be good to expose Length

@Madgvox
Copy link
Contributor Author

Madgvox commented Aug 27, 2024 via email

Renamed NuidWriter to Nuid across the codebase for clarity and brevity. Updated all relevant method calls and references to reflect the new naming convention.
@mtmk
Copy link
Collaborator

mtmk commented Sep 4, 2024

Renamed the class to Nuid and made TryWriteNuid internal again. we can open it up if there is need. plus xml docs. @Madgvox @jasper-d let me know if you're happy then I shall merge. thank you 💯

Copy link
Contributor

@jasper-d jasper-d left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, did not review workflow changes.

src/NATS.Client.Core/Nuid.cs Outdated Show resolved Hide resolved
@Madgvox
Copy link
Contributor Author

Madgvox commented Sep 4, 2024

LGTM 👍

mtmk and others added 2 commits September 4, 2024 19:04
Co-authored-by: Jasper <jasper-d@users.noreply.github.com>
Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM thanks @Madgvox and @jasper-d

@mtmk mtmk merged commit 426ea8c into nats-io:main Sep 4, 2024
10 checks passed
@Madgvox Madgvox deleted the nuidwriter-public branch September 4, 2024 23:57
mtmk added a commit that referenced this pull request Sep 11, 2024
* Fixed consume pending message calculation
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
@mtmk mtmk mentioned this pull request Sep 11, 2024
mtmk added a commit that referenced this pull request Sep 11, 2024
* Fixed consume pending message calculation
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
divyeshio pushed a commit to divyeshio/nats.net that referenced this pull request Sep 13, 2024
* Fixed consume pending message calculation
* ServiceProvider callback for NATS DI configuration (nats-io#619)
* Nats web socket opts improvements (nats-io#623)
* Fix various disposable issues (nats-io#625)
* Make NuidWriter public (nats-io#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (nats-io#605)
mtmk added a commit that referenced this pull request Sep 17, 2024
* Fixed consume pending message calculation (#626)
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
* Simplified NATS client (#607)
* Update docs (#595)
* Add default timeout to initial commands (#594)
* Extensive logging for reconnect debugging (#593)
* Add clear next step navigation to API index doc (#592)
* Add NATS client implementation (#589)
@mtmk mtmk mentioned this pull request Sep 17, 2024
mtmk added a commit that referenced this pull request Sep 17, 2024
* Fixed consume pending message calculation (#626)
* ServiceProvider callback for NATS DI configuration (#619)
* Nats web socket opts improvements (#623)
* Fix various disposable issues (#625)
* Make NuidWriter public (#618)
* NatsOpts.ConfigureWebSocketOpts callback handler (#605)
* Simplified NATS client (#607)
* Update docs (#595)
* Add default timeout to initial commands (#594)
* Extensive logging for reconnect debugging (#593)
* Add clear next step navigation to API index doc (#592)
* Add NATS client implementation (#589)
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.

Make the NuidWriter class public
3 participants