Skip to content

Update install.py - #5173

Closed
Avinash-Raj wants to merge 1 commit into
nodejs:masterfrom
Avinash-Raj:patch-1
Closed

Update install.py#5173
Avinash-Raj wants to merge 1 commit into
nodejs:masterfrom
Avinash-Raj:patch-1

Conversation

@Avinash-Raj

Copy link
Copy Markdown

-> Why I have changed s = re.sub(r'#.*?\n', '', s) to s = re.sub(r'#.*\n?', '', s)?

Because your regex won't match the comment line which exists at the last (End of a file) because it expects a newline character . But s = re.sub(r'#.*\n?', '', s) should remove all the comment lines irrespective of their locations. .*? -> .* because dot by default won't match line breaks.

-> A simple string.replace should do this re.sub(r'\'', '"', s) job, you don't need to go for `re.sub

If you want to further reduce these two lines of code into a single line then go with this,

re.sub(r"#.*\n?|(')", lambda x: '"' if x.group(1) else '', s)

@rvagg

Copy link
Copy Markdown
Member

Is there a particular case you have where there is a comment at the bottom of the file? The change lgtm and seems more correct (.*? is pretty amusing) but I'm wondering if it's addressing an actual problem.

@mscdexmscdex added the tools Issues and PRs related to the tools directory. label Feb 10, 2016
@estliberitas
estliberitasforce-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompareApril 26, 2016 05:23
@Fishrock123

Copy link
Copy Markdown
Contributor

Closing, @Avinash-Raj if you can show somewhere that could be affected by this we'll re-open. :)

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

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Avinash-Raj@rvagg@Fishrock123@mscdex