Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

R11baka
Copy link

@R11baka R11baka commented May 31, 2024

✔ Checklist:

  • Commit messages have been written in Conventional Commits format;
  • Tests for the changes have been added (for bug fixes / features);
  • Docs have been added / updated on the dotenv-linter.github.io (for bug fixes / features).

Description:

Fixes bug with alternate names. #776

@R11baka R11baka changed the title fix(749): allow alternate syntax for bash variables fix(776): allow alternate syntax for bash variables Jun 2, 2024
Copy link
Member

@mgrachev mgrachev left a comment

Choose a reason for hiding this comment

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

👋 Thank you for your contribution 👍

I have left a comment. Please take a look at it 👀

dotenv-linter/src/checks/substitution_key.rs Show resolved Hide resolved
@mgrachev mgrachev linked an issue Jun 3, 2024 that may be closed by this pull request
Co-authored-by: Grachev Mikhail <work@mgrachev.com>
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (5b6de3a) to head (1f2f771).
Report is 13 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #786      +/-   ##
==========================================
- Coverage   99.15%   99.11%   -0.04%     
==========================================
  Files          46       48       +2     
  Lines        2834     3166     +332     
==========================================
+ Hits         2810     3138     +328     
- Misses         24       28       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DDtKey
Copy link
Member

DDtKey commented Jun 3, 2024

Hi and thank you for the contribution!

Just a nitpick: I would consider this as a feature rather than fix.
Because we introduce support for alternative syntax.

dotenv-linter/src/checks/substitution_key.rs Show resolved Hide resolved
Comment on lines +152 to +155
("FOO=${NODE_ENV:development}", None),
("BAR=${CI:+only}", None),
("BUS=${var:=DEFAULT}", None),
("FOOBAR=${var:?IS_UNSET}", None),
Copy link
Member

@DDtKey DDtKey Jun 3, 2024

Choose a reason for hiding this comment

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

Btw, @mgrachev do you think it's ok just to allow all these chars?

#776 syntax still has some rules, while we may encounter something like: FOO=${SOME_VALUE--$$++???_END} - which is invalid and moreover is buggy. It will substitute SOME_VALUE and preserve remaining part

I don't think it's really good idea. Allowing alternative syntax, we also marks invalid ones as valid.
Proper solution would require some kind of parser here

Copy link
Member

Choose a reason for hiding this comment

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

You're right, we definitely shouldn't skip such values: FOO=${SOME_VALUE--$$++???_END}.

Perhaps we can check such values using a regular expression of the form \:[\+\=\?\-]??

Copy link
Member

@DDtKey DDtKey Jun 4, 2024

Choose a reason for hiding this comment

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

Regex may do the trick.

However, need to keep in mind that we also need to ensure it's only 1 time occurrence per substitution, and not something like:
${SOME_VALUE:+one:-two:?three}

It still possible ofc, but probably we can just go with simpler solution:

  • pre-define the list of allowed sequences, e.g: [":+", ":-", ":=", ":?"]
  • ensure there is only one occurrence within the value

Also, need to ensure it will work with multiple substitutions correctly, e.g:

  • positive test: FOO=${NODE_ENV:development}${CI:+only}
  • negative test: FOO=${NODE_ENV:development}${CI:+only:?IS_UNSET:other}`

Copy link
Member

Choose a reason for hiding this comment

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

Good idea 👍

dotenv-linter/src/checks/substitution_key.rs Show resolved Hide resolved
@DDtKey
Copy link
Member

DDtKey commented Jul 22, 2024

Hi @ro0NL 👋

First of all, requested changes must be applied: #786 (comment)

Otherwise the PR will introduce another issue.

@R11baka any chance you will take a look / work on this? Just to understand the state of PR

Thanks everyone 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Expand syntax support for variable default and alternate values

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.