Skip to content

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Apr 30, 2025

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:

  • borrow needs_quotes
  • borrow escape
  • Add E2E test using python-dotenv, where we load a correct multiline/special-character env file into a dict, dump it back and assert it is identical
  • Use snapshot tests for the multiline fixtures

closes #190

dump_env/cli.py Outdated
from dump_env.exceptions import StrictEnvError


def needs_quotes(value: str) -> bool:
Copy link
Contributor Author

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.

Copy link
Contributor Author

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:
Copy link
Contributor Author

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

@joscha joscha Apr 30, 2025

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.

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

to avoid:

MY_VAR==

Copy link
Contributor Author

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

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

}, indent=4))

variables = delegator('dump-env -p MULTILINE_')
assert variables == '''VALUE="{
Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Please, use syrupy!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in c2c8e94

Copy link
Member

@sobolevn sobolevn left a 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.

}, indent=4))

variables = delegator('dump-env -p MULTILINE_')
assert variables == '''VALUE="{
Copy link
Member

Choose a reason for hiding this comment

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

Please, use syrupy!

@joscha
Copy link
Contributor Author

joscha commented May 1, 2025

Thanks! I like the idea, this would need a bit more work + more tests.

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?

@sobolevn
Copy link
Member

sobolevn commented May 6, 2025

Mostly tests :)

@joscha joscha force-pushed the joscha/multiline-support branch from 4643b69 to c2c8e94 Compare May 13, 2025 13:55
@joscha
Copy link
Contributor Author

joscha commented May 13, 2025

@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 ?

@joscha joscha requested a review from sobolevn May 13, 2025 14:44
@sobolevn
Copy link
Member

Yes, let's add ruff and pre-commit 🤝

@joscha
Copy link
Contributor Author

joscha commented May 14, 2025

Yes, let's add ruff and pre-commit 🤝

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?

@joscha
Copy link
Contributor Author

joscha commented May 14, 2025

Yes, let's add ruff and pre-commit 🤝

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"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned by @sobolevn in #387

# 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned in #387

Copy link

codecov bot commented May 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6bd75f4) to head (4231391).
⚠️ Report is 100 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joscha
Copy link
Contributor Author

joscha commented May 15, 2025

@sobolevn I think this is ready

Copy link
Member

@sobolevn sobolevn left a 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! 🤝

joscha and others added 4 commits May 18, 2025 18:41
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: sobolevn <mail@sobolevn.me>
Co-authored-by: sobolevn <mail@sobolevn.me>
Copy link
Member

@sobolevn sobolevn left a 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 :)

@sobolevn sobolevn merged commit 7e5fc14 into wemake-services:master May 18, 2025
8 checks passed
@joscha joscha deleted the joscha/multiline-support branch May 19, 2025 07:46
@joscha
Copy link
Contributor Author

joscha commented May 19, 2025

This was released in 1.6.0 (thank you @sobolevn)

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.

Multi-line support

2 participants