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

chore: ApiBuilder should support Default without a token #60

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

polarathene
Copy link

When creating an API instance the current methods enforce checking for a local token file, even when you intend to provide your own via with_token(), thus finding a token is redundant, as is the info log of one missing being emitted.

  • from_cache() calls cache.token() to set the token field in the builder.
  • You cannot call with_token() prior, both default() and new() methods use the same initialization. Only workaround is to duplicate from_cache() and omit the token check.
  • The token is later used by the private build_headers() method during the final build() call.

This PR changes:

  • The Default impl for both sync/async structs to provide the same struct value except for the token field, leaving it as None by default. from_cache() will use the "Struct Update" syntax to spread the remaining default field values (this communicates more clearly what the method is actually modifying).
  • The order of fields for the Api and ApiBuilder structs were grouped contextually for better visibility vs interleaved. Handled via a separate commit.

I've tried to minimize affecting anything else, but technically if someone was relying on ApiBuilder::default() before instead of ::new(), the only difference should be that token is not potentially set for them. Anyone who is using ::new() should be unaffected by this change.


Related: #54 (comment)

The `new()` method would call `from_cache()` and enforce an attempt to retrieve the token from a local file, even when the builder will be provided with an explicit token.

Instead allow a default where the token initializes as `None` without redundantly logging a missing token file.
@polarathene polarathene changed the title chore: ApiBuilder support default without token chore: ApiBuilder should support Default without a token May 19, 2024
@Narsil
Copy link
Collaborator

Narsil commented Dec 25, 2024

I do not see the issue with that extra logging.

There is effectively no token in the Cache folder, so the log seems quite correct. It's also INFO level since there's nothing wrong with this at all.
Why not adding a log when you are setting the token afterwards ?

That or setting the token file directly in your cache folder before passing it to from_cache ?.

I would remove the logging statement before doing anything else to be honest if that's the real issue.

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