FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

BUGFIX: 627 fix include order by DanielSiepmann · Pull Request #628 · gitpython-developers/GitPython · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

BUGFIX: 627 fix include order - #628

Closed
DanielSiepmann wants to merge 2 commits into
gitpython-developers:masterfrom
DanielSiepmann:hotfix/627-fix-include-order
ConversationCommits (2)ChecksFiles changed
Closed

BUGFIX: 627 fix include order#628
DanielSiepmann wants to merge 2 commits into
gitpython-developers:masterfrom
DanielSiepmann:hotfix/627-fix-include-order

Conversation

DanielSiepmann commented May 14, 2017 •
edited
Loading

Copy link
Copy Markdown
  • 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

BUGFIX: Fix include order of config files
4f77888 
* 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
BUGFIX: Fix order of included files
52ea999

DanielSiepmann commented May 14, 2017

Copy link
Copy Markdown
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.

Byron requested changes May 28, 2017
View reviewed changes

Byron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread git/config.py
files_to_read.append(include_path)
include_paths.append(include_path)
num_read_include_files += 1
include_paths.reverse()

Byron May 28, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

yarikoptic mentioned this pull request Nov 27, 2017
Merged
3 tasks done
Byron closed this Jan 12, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Byron Byron requested changes

Assignees

No one assigned

Labels

waiting for feedback

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL