-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added clarification about default config_filename in Limiter #170
base: master
Are you sure you want to change the base?
Conversation
@@ -56,6 +56,11 @@ You can always switch this during the lifetime of the limiter: | |||
limiter.enabled = False | |||
``` | |||
|
|||
#### Note: config file is set to ".env" by default. Therefore, if you have a .env file setup for something else already, specify the config_filename parameter |
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 is the only section using 4# - - use use 2## to keep it consistent?
Consider moving this lower, to line 74, to keep it near another customization
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.
Also maybe use a shorter section title, like "Using the config file" and set your comment above as the paragraph content?
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.
Hello @FloareDor thank you for the PR, docs are always useful!
I was about to suggest that you update the API reference page with this info as well, but the default value is already mentioned there.
The docs don't actually mention the config file anywhere, except in the API reference, which I think is not obvious enough. Do you think we should add something on the front page, under "Features" for instance?
I'm also wondering about the actual issue you encountered, is there something else than just the .env
value that caused your problem?
@@ -56,6 +56,11 @@ You can always switch this during the lifetime of the limiter: | |||
limiter.enabled = False | |||
``` | |||
|
|||
#### Note: config file is set to ".env" by default. Therefore, if you have a .env file setup for something else already, specify the config_filename parameter |
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.
Also maybe use a shorter section title, like "Using the config file" and set your comment above as the paragraph content?
Description:
I encountered a UnicodeDecodeError related to the default config_filename in SlowAPI's Limiter function. After investigating SlowAPI's source code, I found that '.env' is used as the default config_filename. This PR aims to prevent similar issues by documenting this default behavior.
Please review and consider merging.
Thank you for maintaining this project.
Regards,
FloareDor