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

default log level and tokens #37

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

danchalmers
Copy link

@danchalmers danchalmers commented Jul 24, 2023

The logging within the api code should not fix the logging config in a way that is hard to overwrite, for code which will import it. As discussed in #35

This PR does a couple of logging related things things:

  1. It extracts the logging config out to one place in the API code.
  2. It reduces that config so that it is easier to overwrite in any importing code.
  3. It updates the DEMO notebook, with a couple of alternative ways of changing the logging, and noting that the default has changed.

The log level can still be changed but is not overridden by this code.

The default log level is now WARNING. This makes the default much less verbose. It can easily be changed, as was already documented. Will this change confuse users?

In testing and tweaking the demo I also hit an issue with the token storage in a fresh clone of the API:

  • bcr-authenticate had a default tokens file of ~/.bcr/credentials.txt
  • BWProject and BWUser had a default location of ./tokens.txt
  • The notebook moves from bcr-authenticate to providing tokens.txt in example calls, when that file was non-existent or empty and so led to errors.

A missing user in the tokens file also led to an unhelpful exception message, which I've addressed.

I've made BWProject and BWUser share the same default as bcr-authenticate.
If code has been written assuming tokens.txt rather than explicitly providing it this will break.
The DEMO notebook has been updated to comment on this, but for existing code this may be a breaking change.

It would be easy to make tokens.txt the default, but I assume that a central location was introduced for a reason.

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.

1 participant