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 SCM replacement with comments below it #4580

Merged
merged 6 commits into from Feb 26, 2019

Conversation

Projects
None yet
2 participants
@jgsogo
Copy link
Member

commented Feb 20, 2019

Changelog: Fix: SCM replacement with comments below it
Docs: omit

closes #4531

Parser and substitution succeed, but there are two pending tasks (failing tests):

  • comment string is lost

  • multiline comments fails

  • errors related to encoding/shebang if we load again the class

  • multiline comments get lost after parsing

@ghost ghost assigned jgsogo Feb 20, 2019

@ghost ghost added the stage: review label Feb 20, 2019

@jgsogo jgsogo force-pushed the jgsogo:issue/4531-scm-replacement branch 2 times, most recently from 3160f83 to c31bf21 Feb 20, 2019

@jgsogo jgsogo added this to the 1.13 milestone Feb 20, 2019

@jgsogo jgsogo force-pushed the jgsogo:issue/4531-scm-replacement branch from c31bf21 to d1f9afb Feb 20, 2019

@jgsogo jgsogo force-pushed the jgsogo:issue/4531-scm-replacement branch from 0c72c64 to 0dee220 Feb 21, 2019

@@ -203,16 +204,22 @@ def _replace_scm_data_in_conanfile(conanfile_path, scm_data):
next_line = tree.body[i_body+1].lineno - 1
else:
next_line = statements[i+1].lineno - 1
next_line_content = lines[next_line].strip()
if next_line_content.endswith('"""') or next_line_content.endswith("'''"):
next_line += 1

This comment has been minimized.

Copy link
@jgsogo

jgsogo Feb 21, 2019

Author Member

This is a hack to handle multiline comments...

This comment has been minimized.

Copy link
@lasote

lasote Feb 26, 2019

Contributor

Yeah, pretty weird, but leave it.

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

Please, review, it is very important to add a test with any weird use-case you can think of as the code selecting the lines to replace in the original file might not be as clear as it should be.

Right now, the only issue is that we remove multiline comments that are just below the SCM attribute :/

@jgsogo jgsogo requested a review from memsharded Feb 21, 2019

@lasote

lasote approved these changes Feb 26, 2019

@@ -203,16 +204,22 @@ def _replace_scm_data_in_conanfile(conanfile_path, scm_data):
next_line = tree.body[i_body+1].lineno - 1
else:
next_line = statements[i+1].lineno - 1
next_line_content = lines[next_line].strip()
if next_line_content.endswith('"""') or next_line_content.endswith("'''"):
next_line += 1

This comment has been minimized.

Copy link
@lasote

lasote Feb 26, 2019

Contributor

Yeah, pretty weird, but leave it.

@lasote lasote merged commit a239c88 into conan-io:develop Feb 26, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Feb 26, 2019

@lasote

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I'm ok with the corner cases.

@lasote lasote removed the request for review from memsharded Feb 26, 2019

@jgsogo jgsogo deleted the jgsogo:issue/4531-scm-replacement branch Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.