-
Notifications
You must be signed in to change notification settings - Fork 21
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
Set lower default verbosity and add config options. #150
Conversation
I think this shoudl cover most of the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks @Carreau. Just some minor comments on the documentation.
docs/configuration.md
Outdated
## Build logging levels | ||
|
||
Jupyterlite-sphinx exposes jupyterlite debug and logging flags thought the | ||
following configuration options. | ||
|
||
|
||
- `jupyterlite_debug`, boolean (`False`|`True`), passes the `--debug` flag to | ||
`jupyterlite build` | ||
|
||
It also has the following configuration options: | ||
|
||
- `jupyterlite_log_level`, a `str`, default to `"WARN"` or `None` | ||
- `jupyterlite_verbosity` a `str`, default to `"0"` or `None` | ||
- `jupyterlite_reporter` a `str`, default to `"zero"` or `None` | ||
|
||
Jupyterlite-sphinx decreases verbosity by deafult, these can be set to `None` to restore the jupyterlite default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should mention verbosity controls the verbosity setting of doit
which is used in jupyterlite
, and link to the doit
config docs, https://smarie.github.io/python-doit-api/api_reference/.
And also mention that jupyterlite_log_level
controls the log_level
for jupyterlite build
itself. I can't find --log-level
in the jupyterlite
docs though.
It's also unclear to me what "default to "Warn"
or "None"
" etc. means
Maybe just "defaults to "Warn"
etc.? And then just mention that setting these values to None
gives the defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I get it now. i think I'd understand if it was something like
a str
(defaults to "WARN"
) or None
I fixed the docs and merged it since I want to get this in ASAP since its causing serious trouble in SciPy. I plan to make a release now. |
This resolves Issue #106 as this changes the expected type from |
This also breaks builds as there is now no longer a |
for c in ( | ||
"jupyter", | ||
"lite", | ||
"doit" "build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Carreau I think you missed a comma here(?).
I don't have time now, but I will try to debug this more tonight US time.
Many apologies everyone. It looks like I jumped the gun. @matthewfeickert, I’m away from my machine and on mobile, but can take a look at things too in a couple hours. |
Thanks @steppi. As |
jupyterlite_dir, | ||
"--log-level" if jupyterlite_log_level is not None else None, | ||
jupyterlite_log_level, | ||
"--", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this --
is also an unintended addition. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.f. PR #152
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's needed to pass the args to doit
in jupyterlite build
. See jupyterlite/jupyterlite#1351
Yes. That's definitely needed. I think one of @martinRenou or @jtpio will need to yank it though. |
Sorry I should have tested more. I mostly checked it was not failing at the end of yesterday. Thanks for caching that,. |
I think this should cover most of the cases.
See #149