Skip to content

BUGFIX: 627 fix include order #628

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

Closed
wants to merge 2 commits into from
Closed

BUGFIX: 627 fix include order #628

wants to merge 2 commits into from

Conversation

DanielSiepmann
Copy link

@DanielSiepmann DanielSiepmann commented May 14, 2017

  • Fix order of parsing configured include files.
  • More specific files should overrule included files.
  • Include files at same level where include directive happens.

Resolves: #627

* Add include files to the beginning to parse them right after the file
  containing the include directive.
* This way more specific config files will overrule the configurations.

Resolves: #627
@DanielSiepmann
Copy link
Author

There might be a pythonic way of solving this, but as I'm a beginner im happy to provide at least a fix for the issue.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

First of all, sorry for letting this PR wait for so long, despite me being super happy about every contribution!
So thanks a lot for taking the time to make it!

Besides that simple improvement in readability, do you think you can figure out how to adjust this test-case to assert for the change this PR is making?

You should be able run the config tests via nosetests git/test/test_config.py from the root of this repository.

If adjusting the test doesn't work for you, after all it's not particularly easy to understand and quite complex/convoluted, please let me know and we find another way.

Thanks and cheers

num_read_include_files += 1
include_paths.reverse()
Copy link
Member

Choose a reason for hiding this comment

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

I think this block should be rewritten like this:

files_to_read = include_paths + files_to_read

This makes clearer that included files should be read and merged first.

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

Successfully merging this pull request may close these issues.

2 participants