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

Adds Make generator #4003

Merged
merged 7 commits into from Nov 28, 2018
Merged

Adds Make generator #4003

merged 7 commits into from Nov 28, 2018

Conversation

@danimtb
Copy link
Member

@danimtb danimtb commented Nov 26, 2018

Changelog: Feature: Added new make generator.

  • Refer to the issue that supports this Pull Request: closes #3773
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.
@danimtb danimtb added this to the 1.10 milestone Nov 26, 2018
@ghost ghost assigned danimtb Nov 26, 2018
@ghost ghost added the stage: review label Nov 26, 2018
@danimtb danimtb changed the title Feature/3773 Adds Make generator Nov 26, 2018
@danimtb
Copy link
Member Author

@danimtb danimtb commented Nov 27, 2018

I will highly appreciate a review of @solvingj as he is the genuine creator of this generator 😄

conans/test/generators/make_test.py Outdated Show resolved Hide resolved
conans/client/generators/make.py Outdated Show resolved Hide resolved
@danimtb danimtb removed their assignment Nov 27, 2018
@ghost ghost assigned danimtb Nov 27, 2018
conans/client/generators/__init__.py Show resolved Hide resolved
conans/client/generators/make.py Outdated Show resolved Hide resolved
conans/client/generators/make.py Outdated Show resolved Hide resolved
@danimtb danimtb removed their assignment Nov 27, 2018
@danimtb danimtb requested a review from jgsogo Nov 28, 2018
Copy link
Member

@jgsogo jgsogo left a comment

Cosmetic changes.


@property
def assignment_append(self):
return " += "
Copy link
Member

@jgsogo jgsogo Nov 28, 2018

Why properties and not plain attributes?

Copy link
Member Author

@danimtb danimtb Nov 28, 2018

I left it as it was. I think it is done that way to prevent any change in the content

"$(CONAN_EXELINKFLAGS_MYPKG1) \\\n"
"$(CONAN_EXELINKFLAGS_MYPKG2)", content)
self.assertIn("CONAN_EXELINKFLAGS_MYPKG1 += \\\n-framework QuartzCore", content)
self.assertIn("CONAN_EXELINKFLAGS_MYPKG2 += \\\n-framework VideoToolbox", content)
Copy link
Member

@jgsogo jgsogo Nov 28, 2018

I prefer to see all the file written and compare with it... all these previous asserts are checking for almost it all, aren't they?

Copy link
Member Author

@danimtb danimtb Nov 28, 2018

ok, I could change it. Took it from the premake test bu I agree it would be easier to read

Copy link
Contributor

@solvingj solvingj Nov 28, 2018

i was thinking about this at end of day yesterday. Please feel free to refactor the operators in any way that makes sense. Looking at the current state of the generator, it's possible you can just inline the "assignment" operator text wherever it's used and eliminate the two properties altogether.

jgsogo
jgsogo approved these changes Nov 28, 2018
@ghost ghost assigned danimtb Nov 28, 2018
@danimtb danimtb removed their assignment Nov 28, 2018
@danimtb danimtb merged commit 32d4508 into conan-io:develop Nov 28, 2018
2 checks passed
@ghost ghost removed the stage: review label Nov 28, 2018
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
* Added Make generator and unit test

* Makefile example

* full integration test

* review fixes

* more review

* unified makefile flags

* compare contents directly and use attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants