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

Use Jinja2 to parse layout files (editable packages) #4596

Merged
merged 55 commits into from Feb 27, 2019

Conversation

Projects
None yet
3 participants
@jgsogo
Copy link
Member

commented Feb 22, 2019

Changelog: Feature: Apply Jinja2 to layout files before parsing them
Docs: conan-io/docs#1093

This PR executes Jinja2 over layout files (context contains reference, settings and options) before parsing those files so the user will be able to apply some logic on the constructed path to match the underlying filesystem. As this logic depends on settings and options we are no longer able to parse these files before knowing the node/settings/options it is going to be applied to.

closes #4424

Running on more python versions as we are introducing a new dependency:
@PYVERS: Macos@py27, Windows@py36, Linux@py27, py34

@ghost ghost assigned jgsogo Feb 22, 2019

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

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

@lasote lasote requested a review from memsharded Feb 26, 2019

@lasote lasote self-requested a review Feb 26, 2019

@lasote
Copy link
Contributor

left a comment

Looks good. Don't forget the docs. I want @memsharded to review this, as he is more familiar with this code.

Show resolved Hide resolved conans/model/editable_cpp_info.py Outdated
def load(filepath):
parser = configparser.ConfigParser(allow_no_value=True)
parser.optionxform = str
def _work_on_item(value):

This comment has been minimized.

Copy link
@lasote

lasote Feb 26, 2019

Contributor

Maybe now it deserves a new name or to disappear.

jgsogo added some commits Feb 26, 2019

@memsharded
Copy link
Contributor

left a comment

This PR is difficult to review. Many tests changed, too many refactors and things moved. It is difficult to know if some other functionality rather than introducing Jinja2 has been changed.

It would be better to keep functional PR to the minimum changeset possible, and then refactor later in a pure refactor PR.

Show resolved Hide resolved conans/test/functional/editable/link_create_test.py Outdated
@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

These are just the needed changes:

  • Functionality: it all is in the class EditableLayout. Introducing the Jinja2 template engine requires modifying some logi: we can't parse the file in advance, we need to wait until the last moment to know settings and options (it is not a refactor, it is needed).
  • Tests:
    • functional/integration: only minor changes to adopt the new format
    • unittests: one function <-> one class::function, not so hard to follow:
      • apply_test.py ==> EditableLayout::apply_to
      • load_data_test.py ==> EditableLayout::_load_data (focusing on field errors)
      • parse_test.py ==> EditableLayout::_load_data (focusing on render+parsing)
      • work_on_items_test.py ==> Removed, as there is nothing to test now, I agree with @lasote and I would remove this function

If the functionality has changed, the unittest change, there is no diff suitable for them.

@memsharded
Copy link
Contributor

left a comment

My previous review was exaggerated, was just the movement of ParseTest, nothing else.
Good job, can be merged!

Show resolved Hide resolved conans/requirements.txt Outdated

@lasote lasote merged commit 5093a63 into conan-io:develop Feb 27, 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 27, 2019

@jgsogo jgsogo deleted the jgsogo:issue/4424-add-jinja2 branch Feb 27, 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.