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

Add warnings and errors to ImpactsOutput #99

Merged
merged 13 commits into from
Dec 13, 2024
Merged

Add warnings and errors to ImpactsOutput #99

merged 13 commits into from
Dec 13, 2024

Conversation

samuelrince
Copy link
Member

For #59

@samuelrince samuelrince marked this pull request as ready for review December 7, 2024 14:56
@samuelrince samuelrince changed the title Add warnings and errors to Impacts output Add warnings and errors to ImpactsOutput Dec 7, 2024
Copy link
Member

@adrienbanse adrienbanse left a comment

Choose a reason for hiding this comment

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

Thanks @samuelrince for the nice PR! Much better like this

Just a small comment. Although I completely agree that spamming the user with the logger for the warnings is a bad idea, it still seems to me that the errors should be very easily spotable by the user. A first-time user could try EcoLogits, get nonsense values in ImpactsOutput and miss the error.
--> To me, it should be crystal clear from the welcome page in ecologits.ai that the user has to check himself/herself the errors

@samuelrince
Copy link
Member Author

samuelrince commented Dec 9, 2024

Yes, I agree with you @adrienbanse, more documentation on the get started should be added! I'm thinking, maybe we can also consider showing warnings and errors in the console the first time they appear and then ignoring them. I'll try to improve this!

@samuelrince samuelrince marked this pull request as draft December 9, 2024 09:23
@samuelrince samuelrince marked this pull request as ready for review December 13, 2024 15:56
@samuelrince
Copy link
Member Author

samuelrince commented Dec 13, 2024

Ping @adrienbanse if you want to check the new changes on the logging system :)

FYI I am working on an improved version of the documentation on docs/update branch. It will include more info on errors and warnings on the main page.

Copy link
Member

@adrienbanse adrienbanse left a comment

Choose a reason for hiding this comment

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

This is great, thanks @samuelrince

@samuelrince
Copy link
Member Author

Also, ping @cvarrei, logging behavior will change. Warnings and errors related to the impact calculation will be shown only once, but still present in the ImpactsOutput object on the warnings and errors fields. (related to an old issue #65)

@samuelrince samuelrince merged commit 0cd0df2 into main Dec 13, 2024
2 checks passed
@samuelrince samuelrince deleted the feat/warnings branch December 13, 2024 19:35
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