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

Lint - merge markers - ignore binary files. #1040

Merged
merged 3 commits into from
May 1, 2021

Conversation

ewels
Copy link
Member

@ewels ewels commented Apr 27, 2021

Merge markers lint test:

  • Ignore binary files
  • Added ability to ignore specific files in the lint config.

Moved binary detection function into utils.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

ewels added 2 commits April 27, 2021 11:22
Also add ability to ignore specific files in config.
Moved binary detection function into utils.
@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #1040 (3805eb4) into dev (29a8b5b) will increase coverage by 0.04%.
The diff coverage is 82.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1040      +/-   ##
==========================================
+ Coverage   69.54%   69.59%   +0.04%     
==========================================
  Files          35       35              
  Lines        4344     4354      +10     
==========================================
+ Hits         3021     3030       +9     
- Misses       1323     1324       +1     
Impacted Files Coverage Δ
nf_core/lint/merge_markers.py 87.17% <72.72%> (-0.33%) ⬇️
nf_core/utils.py 83.93% <90.00%> (+0.20%) ⬆️
nf_core/create.py 90.90% <100.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29a8b5b...3805eb4. Read the comment docs.

try:
with io.open(os.path.join(root, fname), "rt", encoding="latin1") as fh:
for l in fh:
if ">>>>>>>" in l:
failed.append(f"Merge marker '>>>>>>>' in `{os.path.join(root, fname)}`: {l}")
failed.append(f"Merge marker '>>>>>>>' in `{os.path.join(root, fname)}`: {l[:30]}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a line be shorter than 30, and wold it break then? 🤔

Small idea although not really necessary, since we're looping through all the lines anyway we could also count them and spit out the line number?

Copy link
Member Author

Choose a reason for hiding this comment

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

No it doesn't break if it's shorter than 30, at least I'm pretty sure it doesn't. Let me check..

python
Python 3.8.6 | packaged by conda-forge | (default, Jan 25 2021, 23:22:12)
[Clang 11.0.1 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> myvar = "123"
>>> myvar
'123'
>>> myvar[:30]
'123'

Yeah it's fine.

Could do line numbers I guess - but meh 🙄 Feel free to add if you want 😅

The reason I changed this is because I had a case where the entire several-MB file was on a single line with no newlines, and the whole thing was dumped into the lint error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright thanks for checking :)

Haha okay yeah I see that can be potentially annoying 😅

@ewels ewels merged commit 14bf78c into nf-core:dev May 1, 2021
@ewels ewels deleted the merge_markers_ignore_binary branch May 1, 2021 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants