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

Recommend adding clarity to envars #79

Closed
dziban303 opened this issue Mar 28, 2023 · 5 comments
Closed

Recommend adding clarity to envars #79

dziban303 opened this issue Mar 28, 2023 · 5 comments

Comments

@dziban303
Copy link
Contributor

dziban303 commented Mar 28, 2023

Many of the variables in the .env file are ambiguous because of several layers of negation in the verbiage, which leads to confusion and frustration about what the settings will accomplish.

# Don't convert special characters
# Disabled: > and <
# Enabled: > and <
DISABLE_UNESCAPE_TEXT=True

Setting to true could mean disabling the conversion of special characters. Or, it could mean the opposite. The juxtaposition of affirmational and negational words makes it unclear. Something clearer would be:

# Controls the conversion of special characters
# True: > and < are converted to &gt; and &lt;
# False: > and < are left as is
ESCAPE_TEXT=True
  • The first line states exactly what the feature does without a negating "Don't".
  • The description of the setting values have the positive setting first,
  • they use the same variables the user will use (true/false rather than 'Enabled' and 'Disabled'), and
  • show exactly what the result of the setting will be.

Finally, the variable itself is changed to remove the DISABLE_, and other negational words or prefixes ('UN'), even if that requires reversing the effect, because it's another unnecessary and ambiguous layer of negation. Rather than unescape, it should just be escape.

Another example, REPLACE_SUBREDDIT= makes more sense than DISABLE_REPLACE_SUBREDDIT=, especially because setting this particular feature to DISABLE_REPLACE_SUBREDDIT=True seems to actually enable the replacement of /r/whatever with a clickable link. That's the opposite of what the variable name itself suggests! Really the variable should be something like CREATE_SUBREDDIT_LINK, where setting to true leaves no doubt about what will happen: a subreddit link will be created.

@TheLovinator1
Copy link
Owner

TheLovinator1 commented Mar 28, 2023

Yeah, the config file just got worse and worse, lol. When people contacted me on Discord for features it was easiest to add the feature and then 10 minutes later responding with "You can do that by adding APPEND_USERNAME=True"

I am planning to rewrite/reword all the variables and add images, so it should hopefully get better.

I am also planning to add so you can format the message yourself instead of having 10 environment variables that change one thing.

@TheLovinator1
Copy link
Owner

I updated the .env.example, what environment variables should be renamed? What comments should be improved? I will add images showing what each variable does someday.

@dziban303
Copy link
Contributor Author

dziban303 commented Mar 31, 2023

Hey, just looked over the new format. This is great, a huge improvement. Though when I tried to run it with just the defaults of the new .env (excepting my token and a webhook/rule) it threw an error:

WARN[0000] The "LOG_LEVEL" variable is not set. Defaulting to a blank string.
Traceback (most recent call last):
  File "/discord_twitter_webhooks/main.py", line 8, in <module>
    from discord_twitter_webhooks import get, reddit, remove, replace, settings
  File "/discord_twitter_webhooks/get.py", line 10, in <module>
    from discord_twitter_webhooks import settings
  File "/discord_twitter_webhooks/settings.py", line 9, in <module>
    setup_logger()
  File "/discord_twitter_webhooks/logger.py", line 16, in setup_logger
    logger.add(
  File "/opt/pysetup/.venv/lib/python3.11/site-packages/loguru/_logger.py", line 895, in add
    levelno = self.level(level).no
              ^^^^^^^^^^^^^^^^^
  File "/opt/pysetup/.venv/lib/python3.11/site-packages/loguru/_logger.py", line 1522, in level
    raise ValueError("Level '%s' does not exist" % name) from None
ValueError: Level '' does not exist

So perhaps the logic that stuffs a default value if no discrete value (or an invalid value?) is supplied in .env should be looked at. Setting LOG_LEVEL to one of the acceptable values resolves the issue.

Thank you again for making this change, I realize it was probably enough work to be annoying. Really appreciate it 💗

TheLovinator1 added a commit that referenced this issue Mar 31, 2023
@TheLovinator1
Copy link
Owner

TheLovinator1 commented Mar 31, 2023

It should be fixed now, I forgot to set the default value when LOG_LEVEL is equal to nothing.

I realize it was probably enough work to be annoying

The only thing that would happen if the issue/feature is too annoying is that I will be too lazy to fix/add it :p

@dziban303
Copy link
Contributor Author

Closed as completed

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

No branches or pull requests

2 participants