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

Remove inline comments during line preprocessing #161

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

IniasP
Copy link
Contributor

@IniasP IniasP commented Oct 4, 2020

Looks like the current implementation only removes lines that start with "#" to filter comments.
As described in issue #160, inline comments are missed. This should resolve issue #160.

src/parse/utilities.ts Outdated Show resolved Hide resolved
@NilsJPWerner
Copy link
Owner

NilsJPWerner commented Oct 8, 2020

Thank you for opening this PR! Could you add a test to validate that the behavior is correct?

Also fix the handling of lines like "    # comment" by trimming before the startsWith("#") check.
We can't just map once to the trimmed version, since the filter needs the trim before its check and the map can only trim after splitting on "#".
@IniasP
Copy link
Contributor Author

IniasP commented Oct 13, 2020

Good thing you asked for a test case, turns out we missed something (see commit message on 100adc3). Should be good now.

@NilsJPWerner
Copy link
Owner

Sorry for the massive delay on responding to this. Does this handle strings that have the pound sign in them? e.g. x = "# abc"

@@ -15,8 +15,8 @@ export function getIndentation(line: string): string {
*/
export function preprocessLines(lines: string[]): string[] {
return lines
.map(line => line.trim())
.filter((line) => !line.startsWith("#"));
.filter((line) => !line.trim().startsWith("#"))
Copy link
Owner

Choose a reason for hiding this comment

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

We can't do this unfortunately because it will cut off strings with # in them. See previous discussion here: #144

@NilsJPWerner
Copy link
Owner

So we could possibly limit this to only trim comments in the docstring definition since it is less likely to have a string with a # in it than in the body.

@IniasP
Copy link
Contributor Author

IniasP commented Mar 10, 2021

I don't think I realized at the time that hash symbols in strings are possible. I agree with the discussion at #144 that this will be impossible to do exactly right with regex, because it's fundamentally outside of the scope of regular languages.
Also, this is really low impact so maybe this should just be a wontfix thing.
Maybe there should be a test case addressing this specifically, because I did get all tests to pass.

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.

3 participants