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

fix: if a line contains multiple # characters, there will be issues w… #238

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

astak16
Copy link

@astak16 astak16 commented Dec 15, 2024

When traversing from back to front, an error occurs if a line contains multiple # characters, such as bar=foo # baz # other. I changed it to traverse from front to back instead.

Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

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

I was a bit afraid this PR could lead to breaking changes.

But as you said code currently return an error.

So I hope, there is code out there relying on the behavior that an error was returned.

So my confidence level with this PR is about 80 % it would be OK

// a comment (ie asdasd # some comment)
for i := endOfVar - 1; i >= 0; i-- {
if line[i] == charComment && i > 0 {
// a comment (ie asdasd # some comment # other)

Choose a reason for hiding this comment

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

I would like to suggest this, because what you add increase the confusion

Suggested change
// a comment (ie asdasd # some comment # other)
// a comment (ie `whatever # some comment # other`)

Or any variation

Suggested change
// a comment (ie asdasd # some comment # other)
// a comment, ie: `whatever # some comment # other`

Or

Suggested change
// a comment (ie asdasd # some comment # other)
// a comment, ie: whatever # some comment # other

Because asdasd is unclear. I know the comment is not yours, and you are only moving it. But now, the confusion is increased.

I mean, I'm not even sure if whatever should be whatever , foo=bar

@@ -1,4 +1,6 @@
# Full line comment
qux=thud # fred # other
thud=fred#qux # other

Choose a reason for hiding this comment

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

Because a later refactoring could break expected behavior

I would like you to add this, and test it

Suggested change
thud=fred#qux # other
thud=fred#qux # other
quz=foo#baz # other # more

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.

2 participants