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

Added clarification about default config_filename in Limiter #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FloareDor
Copy link

@FloareDor FloareDor commented Sep 1, 2023

Description:

  • Added documentation for the Limiter function's config_filename parameter.
  • Clarified that the default value is ".env" and provided guidance for specifying it when a different .env file exists.

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

@@ -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
Copy link
Contributor

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

Copy link
Owner

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?

Copy link
Owner

@laurentS laurentS left a 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
Copy link
Owner

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?

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.

None yet

3 participants