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

handle inline comments, and ignore spaces outside quotes #475

Merged
merged 7 commits into from Jul 6, 2023

Conversation

lvanderree
Copy link
Contributor

capable of handling inline comments (outside quotes).

So if you want "#" hashes in your values, place them inside quotes.

Also ignoring spaces outside quotes

@github-actions github-actions bot changed the base branch from main to develop May 30, 2023 15:50
@github-actions
Copy link

Your PR was set to target main, PRs should be target develop
The base branch of this PR has been automatically changed to develop, please check that there are no merge conflicts.

@lvanderree
Copy link
Contributor Author

lvanderree commented May 30, 2023

I noticed related issues that are addressed by this PR:

@coveralls
Copy link

coveralls commented Jun 26, 2023

Coverage Status

coverage: 92.255% (+0.04%) from 92.215% when pulling 09a965a on lvanderree:issue/91-inline-comments into 7f31fca on joke2k:develop.

@sergeyklay
Copy link
Collaborator

@lvanderree I've been looking through your pull request and in principle, everything looks great. I really like what you've done here. However, there seem to be a few builds that failed, notably the code style checks and linters.

Could you please have a look into why these didn't pass and make the necessary adjustments? It would greatly assist in maintaining our codebase's quality and readability.

Also, I would appreciate it if you could add an entry in the changelog about these changes. This would save me some time and ensure that we keep our documentation up to date.

Copy link
Collaborator

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

Great work on the PR! Noticed some linter warnings. Please, could you address them? After that, it'll be ready for merge. Thanks!

CHANGELOG.rst Outdated Show resolved Hide resolved
environ/environ.py Outdated Show resolved Hide resolved
environ/environ.py Outdated Show resolved Hide resolved
tests/test_env.py Outdated Show resolved Hide resolved
@sergeyklay
Copy link
Collaborator

Looks great. Thank you for contributing and patience

@sergeyklay sergeyklay merged commit 2750dd5 into joke2k:develop Jul 6, 2023
26 checks passed
@sergeyklay sergeyklay self-assigned this Jul 14, 2023
@sergeyklay sergeyklay added the enhancement New feature or request label Jul 14, 2023
@jonnyhsu
Copy link

jonnyhsu commented Sep 6, 2023

So if you want "#" hashes in your values, place them inside quotes.

Unfortunately the Django SECRET_KEY has "#" as a valid character, so this is actually a fairly serious breaking change.

@lvanderree
Copy link
Contributor Author

Unfortunately the Django SECRET_KEY has "#" as a valid character, so this is actually a fairly serious breaking change.

what is the breaking change exactly? Have you got an example that isn't working anymore?
As far as a I see everything is still possible, but with the additional functionality to also be able to place comments

@sergeyklay
Copy link
Collaborator

sergeyklay commented Sep 8, 2023

.env file contents:

SECRET_KEY_499_1=abc#def
SECRET_KEY_499_2='abc#def'
SECRET_KEY_499_3='abc\#def'
SECRET_KEY_499_4="abc\#def"
SECRET_KEY_499_5=abc\#def

Test 2750dd5 (this PR)

SECRET_KEY_499_1=abc
SECRET_KEY_499_2=abc#def
SECRET_KEY_499_3=abc\#def
SECRET_KEY_499_4="abc\
SECRET_KEY_499_5=abc\

Test 8874288 (v0.10.0)

SECRET_KEY_499_1=abc#def
SECRET_KEY_499_2=abc#def
SECRET_KEY_499_3=abc\#def
SECRET_KEY_499_4=abc#def
SECRET_KEY_499_5=abc\#def

For more see: #499

@sergeyklay
Copy link
Collaborator

Hey @lvanderree,

I'm currently working on some modifications to the functionality you introduced in this PR. While the inline comments feature is useful, it has led to some unintended side effects that need addressing. I've opened a new pull request (see #500) to tackle these issues.

I'd appreciate your thoughts and any feedback you might have on this.

sergeyklay added a commit that referenced this pull request Sep 10, 2023
Disabled inline comments handling (that was introduced in #475)
by default due to potential side effects. While the feature itself is
useful, the project's philosophy dictates that it should not be enabled
by default for all users.

This also fix the issue described in the #499.
@jonnyhsu
Copy link

Unfortunately the Django SECRET_KEY has "#" as a valid character, so this is actually a fairly serious breaking change.

what is the breaking change exactly? Have you got an example that isn't working anymore? As far as a I see everything is still possible, but with the additional functionality to also be able to place comments

If the SECRET_KEY variable starts with a # then the site will not load, because a non-blank secret key is required. If the SECRET_KEY contains a #, then the key will be silently shortened which reduces security.

STR_VAR=bar
STR_QUOTED_IGNORE_COMMENT= 'foo' # comment
STR_QUOTED_INCLUDE_HASH='foo # with hash' # not comment

Choose a reason for hiding this comment

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

String "ab#cd" does not work(

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #500

sergeyklay added a commit that referenced this pull request Sep 22, 2023
Disabled inline comments handling (that was introduced in #475)
by default due to potential side effects. While the feature itself is
useful, the project's philosophy dictates that it should not be enabled
by default for all users.

This also fix the issue described in the #499.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants