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 copyright header test on backup files #340

Merged
merged 3 commits into from May 12, 2016

Conversation

@jakobj
Copy link
Contributor

jakobj commented May 11, 2016

As mentioned in #339 the test currently fails when backup files are present. This PR introduces a regular expression match on the filename to exclude such files from the check. Fixes #339.

'#.*',
'.*~',
]
exclude_file_regex = [re.compile(pattern) for pattern in exclude_file_patterns]

This comment has been minimized.

Copy link
@heplesser

heplesser May 11, 2016

Contributor

I think it would be more elegant to have a single regular expression using the or operator, i.e., something like \.#.*|#.*|.*~. Maybe also add .bak.

This comment has been minimized.

Copy link
@jakobj

jakobj May 11, 2016

Author Contributor

Sure, we could do it in a single one. My thinking was that this way it might be easier to add new patterns later? Like the one you suggested.

This comment has been minimized.

Copy link
@jougs

jougs May 11, 2016

Contributor

I agree with @jakobj that having the patterns individually helps readability. But as they are short, maybe they can all be on a single line?

This comment has been minimized.

Copy link
@heplesser

heplesser May 12, 2016

Contributor

To quite a degree, I think, it is a matter of taste: do you want to put the iteration on the Python side or the Regex side? Since most NEST developers are much more versed in Python than in regular expressions, doing it in Python makes sense. On the other hand, I would expect that placing the iteration in a compiled regular expression object will be more efficient. But run-time efficiency is no issue here, so we should choose a coding style most developers will be familiar with.

@heplesser
Copy link
Contributor

heplesser commented May 12, 2016

@jakobj Would you pull recent changes from master into this PR, so that the PEP8 check passes? Once Travis is happy, you have my 👍.

@jakobj
Copy link
Contributor Author

jakobj commented May 12, 2016

I now added .bak to the excluded patterns, but left them separately. I hope that is fine. @jougs if you're also happy, please consider the thumbs up. :)

@jougs
Copy link
Contributor

jougs commented May 12, 2016

The new code looks very nice to me. 👍 from me. Thanks for taking care!

@heplesser heplesser merged commit dc5edb5 into nest:master May 12, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.