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

Adds argparse to enable log to file #2

Merged
merged 12 commits into from
Jul 5, 2022

Conversation

jmarcelomb
Copy link
Contributor

It is not enabled by default but can be for the development phase. I didn't add the debug variable because it is got through the environment variable. Also, I didn't add a version because it required adding the version in a constant or main file, and I didn't know if it was wanted.

@balloob
Copy link
Contributor

balloob commented Jun 10, 2022

How do you plan on using this for development? Output is currently printed on the terminal, right where you can read it?

@jmarcelomb
Copy link
Contributor Author

jmarcelomb commented Jun 10, 2022

I'm using the hass core in my host, and when I start the matter server I append -v to the python3 matter_server.server command.

Yesterday I had a problem when commissioning the device but because I had a lot of MINMDNS spam from my others smart home devices I couldn't see the logs, I used 2> log_file but I was thinking I could add argparse. It is still outputting to the console but also saves to the file, but I can add a console handler if needed.

Also, I can add a --log-level arg.

@agners
Copy link
Collaborator

agners commented Jun 10, 2022

I think its nice to have the option to generate log files, but by default we should log to the terminal, as this is how it will get captured also by the Matter Server add-on.

So, I'd make log file output dependent of --log-file argument. Only write to the log file if that is passed.

The --log-level seems like a good idea. Not sure if --verbose is needed if we have that, I don't have a strong opinion, if you prefer to keep it as a "quick" option, why not.

@jmarcelomb
Copy link
Contributor Author

I put --log-evel but I can put --loglevel and remove also the small -log. I defaulted to info for the code have the same behaviour.

Copy link
Collaborator

@agners agners left a comment

Choose a reason for hiding this comment

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

You also need to use log_level then in some places. Other than that looks good to me. Ideally we should then add log level as a configuration option for the add-on.

@jmarcelomb jmarcelomb requested a review from agners July 2, 2022 10:10
Copy link
Collaborator

@agners agners left a comment

Choose a reason for hiding this comment

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

This still feels a bit wrong to me: Currently when using python3 -m matter_server.server --log-file my.log it won't write logs to the file, only after adding -v it will log. This is not how I would expect it to behave.

IMHO, the -v can be as a shorthand for --log-file debug, but other than that, it should not influence logging.

--log-file should be the only flag to influence if a file handler is used or not

--log-level should influence the log level.

@jmarcelomb
Copy link
Contributor Author

You are right, I removed the -v arg.I think right now it has the behavior expected.

matter_server/server/__main__.py Outdated Show resolved Hide resolved
matter_server/server/__main__.py Outdated Show resolved Hide resolved
@agners agners merged commit c2e45da into home-assistant-libs:main Jul 5, 2022
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.

3 participants