Implement LeadingCharacterFixer #259

baile320 · 2020-08-14T01:31:29Z

This will close: #248

I have a couple comments/questions:

When implementing this, I ran into issues with IncorrectDelimiter: it thought, for example, "*FOO=BAR" had an incorrect delimiter (this is ultimately fixed by LeadingCharacterFixer, as expected). IMO, that is not a delimiter at all, since delimiters should occur /between/ other characters, so I added a check for that in IncorrectDelimiterChecker (here: https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9d20ccc745eeab0a2cfbe87e10ee860cR27-R33), and removed an erroneous test.
Depending on the leading characters, it is still possible to get erroneous UnorderedKeys warnings. I'm not sure the best way to handle this (I made no updates to that), and would like thoughts/suggestions on how to fix (if it should be fixed here) or if it's worthy of a separate PR. There's an illustrative example in my integration test (https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9aff8b00dfa64d0bf60b17a8b7fe7c20R4-R22)... as you can see, it throws no UnorderedKeys warnings, although it likely should, since after cleaning the keys are technically out of order (the issue is that the checking order includes the */./1 characters at the beginning of the keys, not the "cleaned" versions of the keys).

✔ Checklist:

This PR has been added to CHANGELOG.md (at the top of the list);
Tests for the changes have been added (for bug fixes / features);
Docs have been added / updated (for bug fixes / features).

Delimiters occur between other characters, not at the beginnings of words, so I updated that and removed a test case (it was also interfering with LeadingCharacterFixer).

mgrachev

@baile320 👋 Thank you for your contribution! 🚀

I have left some comments. Please take a look at them 👀

CHANGELOG.md

src/checks/incorrect_delimiter.rs

src/fixes/leading_character.rs

mgrachev · 2020-08-14T09:24:15Z

When implementing this, I ran into issues with IncorrectDelimiter: it thought, for example, "*FOO=BAR" had an incorrect delimiter (this is ultimately fixed by LeadingCharacterFixer, as expected). IMO, that is not a delimiter at all, since delimiters should occur /between/ other characters, so I added a check for that in IncorrectDelimiterChecker (here: https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9d20ccc745eeab0a2cfbe87e10ee860cR27-R33), and removed an erroneous test.

Good catch! Thank you! 👍
But I'd like you to write a test for that case for the IncorrectDelimiter.

Depending on the leading characters, it is still possible to get erroneous UnorderedKeys warnings. I'm not sure the best way to handle this (I made no updates to that), and would like thoughts/suggestions on how to fix (if it should be fixed here) or if it's worthy of a separate PR. There's an illustrative example in my integration test (https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9aff8b00dfa64d0bf60b17a8b7fe7c20R4-R22)... as you can see, it throws no UnorderedKeys warnings, although it likely should, since after cleaning the keys are technically out of order (the issue is that the checking order includes the */./1 characters at the beginning of the keys, not the "cleaned" versions of the keys).

@evgeniy-r is already working on that feature (#252) 💪

src/checks/incorrect_delimiter.rs

… into leading-char-fixer

Bug was introduced into IncorrectDelimiterChecker in previous commit where "*FOO-BAR=VALUE" would not have risen a warning. Additionally, LeadingCharacterFixer would not remove all invalid leading chars, just one. This has also been fixed.

src/checks/incorrect_delimiter.rs

… into leading-char-fixer

baile320 · 2020-08-20T14:38:24Z

I merged in the changes from #267 and made some other relatively minor changes related to test names, checks, formatting. I think this PR is ready to go or be re-reviewed! Thanks!

src/fixes/leading_character.rs

Co-authored-by: Grachev Mikhail <work@mgrachev.com>

… into leading-char-fixer

…nv-linter into leading-char-fixer

mgrachev

LGTM 👍

mgrachev · 2020-08-26T12:35:28Z

@baile320 Thanks a lot! You're awesome! ❤️

baile320 added 4 commits August 13, 2020 19:06

implement LeadingCharacterFixer

7e9aadd

fix IncorrectDelimiterChecker

e54ce1f

Delimiters occur between other characters, not at the beginnings of words, so I updated that and removed a test case (it was also interfering with LeadingCharacterFixer).

add integration test for LeadingCharacterFixer

1296f8e

add LeadingCharacterFixer to CHANGELOG.md

da339d9

DDtKey requested a review from a team August 14, 2020 08:45

mgrachev reviewed Aug 14, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/checks/incorrect_delimiter.rs Outdated Show resolved Hide resolved

src/fixes/leading_character.rs Outdated Show resolved Hide resolved

DDtKey mentioned this pull request Aug 14, 2020

IncorrectDelimiter fixer (#246) #258

Merged

3 tasks

baile320 added 4 commits August 14, 2020 05:54

fix typo in CHANGELOG

b2b50c7

add test for *FOO=BAR to incorrect_delimiter

49d2a79

remove unnecessary line

f683b81

extract function remove_leading_char

3c16dc0

baile320 requested a review from mgrachev August 14, 2020 11:10

mgrachev previously approved these changes Aug 14, 2020

View reviewed changes

mgrachev requested a review from a team August 14, 2020 11:13

DDtKey reviewed Aug 14, 2020

View reviewed changes

src/checks/incorrect_delimiter.rs Outdated Show resolved Hide resolved

baile320 added 2 commits August 14, 2020 08:25

Merge branch 'master' of https://github.com/dotenv-linter/dotenv-linter…

966628e

… into leading-char-fixer

Fix bugs related to previous refactor

271f112

Bug was introduced into IncorrectDelimiterChecker in previous commit where "*FOO-BAR=VALUE" would not have risen a warning. Additionally, LeadingCharacterFixer would not remove all invalid leading chars, just one. This has also been fixed.

baile320 dismissed mgrachev’s stale review via 271f112 August 14, 2020 14:42

baile320 requested review from DDtKey and mgrachev August 14, 2020 14:51

add test for remove_all_invalid_leading_chars

7552785

mgrachev reviewed Aug 19, 2020

View reviewed changes

src/checks/incorrect_delimiter.rs Outdated Show resolved Hide resolved

baile320 mentioned this pull request Aug 19, 2020

Update logic for IncorrectDelimiterChecker #267

Merged

2 tasks

baile320 added 4 commits August 20, 2020 09:27

Merge branch 'master' of https://github.com/dotenv-linter/dotenv-linter…

36e22ca

… into leading-char-fixer

rename default passing test

b228611

rename two more tests

cc17d80

added additional checks to fix_warnings_test

d711ef0

mgrachev requested a review from a team August 20, 2020 15:40

mgrachev reviewed Aug 20, 2020

View reviewed changes

src/fixes/leading_character.rs Outdated Show resolved Hide resolved

src/fixes/leading_character.rs Show resolved Hide resolved

baile320 and others added 4 commits August 20, 2020 11:44

Update src/fixes/leading_character.rs

7fef666

Co-authored-by: Grachev Mikhail <work@mgrachev.com>

rename test, period -> dot

79cd8c3

Merge branch 'master' of https://github.com/dotenv-linter/dotenv-linter…

d28b335

… into leading-char-fixer

Merge branch 'leading-char-fixer' of https://github.com/baile320/dote…

3b208dd

…nv-linter into leading-char-fixer

baile320 requested a review from mgrachev August 20, 2020 19:44

mgrachev requested a review from a team August 21, 2020 12:17

mgrachev previously approved these changes Aug 21, 2020

View reviewed changes

mgrachev requested a review from a team August 21, 2020 12:20

mgrachev added this to the v2.2.0 milestone Aug 21, 2020

mgrachev assigned baile320 Aug 21, 2020

Merge branch 'master' into leading-char-fixer

af966f0

mgrachev dismissed their stale review via af966f0 August 23, 2020 10:54

mgrachev approved these changes Aug 23, 2020

View reviewed changes

mgrachev requested a review from a team August 23, 2020 10:55

mgrachev merged commit 538e2be into dotenv-linter:master Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Implement LeadingCharacterFixer #259

Implement LeadingCharacterFixer #259

Uh oh!

baile320 commented Aug 14, 2020 •

edited

Loading

Uh oh!

mgrachev left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgrachev commented Aug 14, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

baile320 commented Aug 20, 2020

Uh oh!

Uh oh!

Uh oh!

mgrachev left a comment

Uh oh!

mgrachev commented Aug 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Uh oh!

Implement LeadingCharacterFixer #259

Implement LeadingCharacterFixer #259

Uh oh!

Conversation

baile320 commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔ Checklist:

Uh oh!

mgrachev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgrachev commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baile320 commented Aug 20, 2020

Uh oh!

Uh oh!

Uh oh!

mgrachev left a comment

Choose a reason for hiding this comment

Uh oh!

mgrachev commented Aug 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

baile320 commented Aug 14, 2020 •

edited

Loading

mgrachev commented Aug 14, 2020 •

edited

Loading