-
-
Notifications
You must be signed in to change notification settings - Fork 15
feat: multiline & special character support #385
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
feat: multiline & special character support #385
Conversation
dump_env/cli.py
Outdated
from dump_env.exceptions import StrictEnvError | ||
|
||
|
||
def needs_quotes(value: str) -> bool: |
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.
it feels like this should be something we can borrow from an ini file generator and/or dotenv itself. If you have a suggestion where we can pull a less naive implementation from, please let me know.
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.
@theskumar excuse the summoning, but do you happen to have a suggestion here by any chance, please?
dump_env/cli.py
Outdated
) | ||
|
||
|
||
def escape(value: str) -> str: |
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.
ditto for naive implementation and possibly borrowing it elsewhere.
dump_env/cli.py
Outdated
""" | ||
return ( | ||
not value | ||
or ' ' in value |
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 needed when we have something like:
SECRET_VAR=" x "
echo "'$SECRET_VAR'"
# results in ' x '
There is currently no test for this, let me know if you want to add one.
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 covered by def80d3 now
dump_env/cli.py
Outdated
not value | ||
or ' ' in value | ||
or '\n' in value | ||
or '=' in value |
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.
to avoid:
MY_VAR==
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 covered by def80d3 now
dump_env/cli.py
Outdated
or '\n' in value | ||
or '=' in value | ||
or '"' in value | ||
or "'" in value |
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.
I am not actually 100% sure this one is needed, as dotenv
itself says for multiline support a "
is needed anyway, see https://github.com/theskumar/python-dotenv?tab=readme-ov-file#multiline-values
tests/test_cli/test_escape.py
Outdated
}, indent=4)) | ||
|
||
variables = delegator('dump-env -p MULTILINE_') | ||
assert variables == '''VALUE="{ |
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.
these fixtures would actually be expressed in a more readable manner via a snapshot test facility, for example https://github.com/syrupy-project/syrupy - let me know if you want me to refactor these to use snapshots instead.
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.
Please, use syrupy
!
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.
done in c2c8e94
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.
Thanks! I like the idea, this would need a bit more work + more tests.
tests/test_cli/test_escape.py
Outdated
}, indent=4)) | ||
|
||
variables = delegator('dump-env -p MULTILINE_') | ||
assert variables == '''VALUE="{ |
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.
Please, use syrupy
!
Okay, can you let me know which parts need more work (apart from tests) and then I'll update this PR and request review again? |
Mostly tests :) |
4643b69
to
c2c8e94
Compare
@sobolevn tests are in, however the amount of lints is staggering and there are a few that were deprecated from your styleguide. I porpose to add ruff in a separate PR, rebase this one and then fix the remaining lints after? I can adopt whatever is in https://github.com/wemake-services/dotenv-linter/blob/master/pyproject.toml ? |
Yes, let's add |
Okay, great, will open a separate PR. Are you happy with the contents of this? As in: is this one mergeable if we get the lints/style sorted? |
done in #387 |
wemake-python-styleguide = "^1.1" | ||
ruff = "^0.11" | ||
|
||
codespell = "^2.2" |
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.
# Only prefix should be changed, other parts should not: | ||
assert dump_result['DJANGO_SECRET_KEY'] == 'test' # noqa: S105 | ||
assert dump_result['SECRET_VALUE'] == 'value' # noqa: S105 | ||
assert dump_result == snapshot |
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.
as mentioned in #387
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #385 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 83 94 +11
Branches 18 17 -1
=========================================
+ Hits 83 94 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@sobolevn I think this is ready |
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.
Just several small nitpicks and we are ready to go! 🤝
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: sobolevn <mail@sobolevn.me>
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.
Great! Thanks a lot!
I will release a new version now :)
This was released in |
This is only a first, naive implementation to start discussion. Once we honed in whether the direction is fine, I will follow up and also fix the current flake8 errors.
Possible improvements:
borrowneeds_quotes
borrowescape
closes #190