-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improve clarity and usability of README for first-time Howitz users #86
Conversation
Otherwise pip might complain about several directories having the same name: howitz venv dir and howitz module dir
Makes it easier to see this info and use it. Also marks this info clearly as an optional one
User testing showed that this info wasn't apparent enough for a new user
Don't assume user has read the run in dev section before reading this section
There was confusion for a first-timer about what this value should be because of "tld"
It shows how to configure alternative connections, yet doesnt require any action for new users when they copy values from the example config file to the actual config file
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
=======================================
Coverage ? 48.98%
=======================================
Files ? 15
Lines ? 835
Branches ? 0
=======================================
Hits ? 409
Misses ? 426
Partials ? 0 ☔ View full report in Codecov by Sentry. |
b13edcf
to
53c29c7
Compare
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.
Set up worked quite smoothly, no complaints there.
I will add some commits containing small typo/article/formulation fixes.
Created a separate issue for this #91 |
README.rst
Outdated
The easiest way to configure Howitz is via a ``toml`` file. | ||
|
||
1. Create an empty ``.howitz.toml`` file in the project root folder. | ||
2. Copy the values from the example config file ``dev-howitz.toml`` to ``.howitz.toml``. |
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 haven't found a good way to mute the logging that happens on startup unfortunately. Maybe we should ship a default logging config that does that, or use a flag to explicitly turn it on.
README.rst
Outdated
|
||
There's a handler "debug" that will copy everything DEBUG or higher to a file | ||
There's a handler ``debug`` that will copy everything DEBUG or higher to a file |
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.
Better: "As for logging, there is a handler"..
dev-howitz.toml
Outdated
level = "ERROR" | ||
|
||
## Comment out and tweak in order to configure logging level for dev or prod | ||
#[logging.formatters.default] | ||
#format = "%(levelname)s %(name)s in %(funcName)s: %(message)s" | ||
# |
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.
With this, the config file is suitable neither for development nor production without major alterations. I'm not sure this is a good idea. @lunkwill42?
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.
If we want a clean separation, the example config file referenced in the README should not be a file called dev-howitz.toml
. howitz.toml.example
could do - then we could keep a howitz.dev.toml.example
or something. Normal folks do not want to start with the latter file, @hmpf @podliashanyk
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 like the idea with multiple example files!
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.
Added howitz.min.toml.example
and howitz.dev.toml.example
and updated the docs.
Is there a need to provide an additional example file for prod? Or do we just rename the dev one to howitz.toml.example
? @hmpf @lunkwill42
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 took a closer look at the "Configuration"-section in the README
. It describes the differences between dev config and prod config, and mentions the example file. So I landed on having a universal extended howitz.toml.example
and a minimal howitz.min.toml.example
.
Updated the README
accordingly.
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.
Everything looks a lot better now - but we need a decision on multiple example config files. We want to satisfy devs without confusing regular users...
dev-howitz.toml
Outdated
level = "ERROR" | ||
|
||
## Comment out and tweak in order to configure logging level for dev or prod | ||
#[logging.formatters.default] | ||
#format = "%(levelname)s %(name)s in %(funcName)s: %(message)s" | ||
# |
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.
If we want a clean separation, the example config file referenced in the README should not be a file called dev-howitz.toml
. howitz.toml.example
could do - then we could keep a howitz.dev.toml.example
or something. Normal folks do not want to start with the latter file, @hmpf @podliashanyk
172c1e1
to
c87e965
Compare
c87e965
to
7692e0a
Compare
howitz.dev.toml.example
Outdated
|
||
[logging] | ||
version = 1 | ||
|
||
[logging.root] | ||
level = "DEBUG" | ||
level = "ERROR" |
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.
Let this one go back to debug since it is for developers.
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 would add this:
[logging.formatters.default]
format = "%(levelname)s %(name)s in %(funcName)s: %(message)s"
[logging.handlers.null]
class = "logging.NullHandler"
[logging.handlers.wsgi]
class = "logging.StreamHandler"
stream = "ext://flask.logging.wsgi_errors_stream"
formatter = "default"
They are not used unless the [logging.loggers.MODULE]
-stuff is also included.
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.
This seems to clutter the minimal file. Don't really see the reason to include it especially since it will not be activated unless [logging.loggers.MODULE]
-stuff is included.
Since this section is mostly filled with general and universal info about the the values in the toml config file
|
||
[howitz] | ||
storage = "./howitz.sqlite3" | ||
devmode = true |
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.
What's devmode
do? Doesn't sound very production-y to me...
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.
devmode
is primarily used for flask app configuration. Values for flask server address and flask storage location will have reasonable dev defaults if devmode
is True
and are empty otherwise. @hmpf are there any other use cases for devmode
in Howitz?
Good point, it shouldn't be set to true in example config without additional changes! Will fix.
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.
We tried adding a cli-flag devmode
to make it very explicit when running for development: debug is set to True as well as some other things. But we stopped trying to support it as a cli-flag and now keep it only in the config-file because making it work with flask --app howitz run
was proving too hard/not worth the effort.
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.
Have added warnings in the README
about checking out the "Config file for production"-section when configuring Howitz for prod.
But I have a mild suspicion that the mentioned "Config file for production"-section is somewhat incomplete. Have created #109
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.
Fine by me - if my assumption about refresh intervals hold true - otherwise, 5 would be a bad default 😆
README.rst
Outdated
The refresh interval for the events table can be changed by adding for example ``refresh_interval = 10`` to | ||
the ``[howitz]``-section or setting the environment variable ``HOWITZ_REFRESH_INTERVAL`` to a new value. | ||
Refresh interval values are in seconds and must be integers. The default value is ``5`` seconds. |
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.
Question: After the NTIE PR was merged, I hope refresh interval refers to often HTMX is set to poll the Howitz backend for NTIE updates, not to an interval of full table refreshes?
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.
Thats right, it refers to the interval for NTIE updates! NTIE PR is already merged #87 so there is no more interval (or code for that matter) for full table refreshes in Howitz anymore. 😌
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.
The naming refresh/poll is still confusing unfortunately. We decided to use word poll for "poll router or interface"-feature, and word refresh for "poll Zino server for NTIE updates"-feature.
I will add a sentence with clarification on it in the README. 🙃
Improvements stem mostly from the user test on @stveit where he was setting up Howitz to run for the first time by following the docs in the
README
.Closes #93