Skip to content

Lint - merge markers - ignore binary files.#1040

Merged
ewels merged 3 commits into
nf-core:devfrom
ewels:merge_markers_ignore_binary
May 1, 2021
Merged

Lint - merge markers - ignore binary files.#1040
ewels merged 3 commits into
nf-core:devfrom
ewels:merge_markers_ignore_binary

Conversation

@ewels

@ewels ewels commented Apr 27, 2021

Copy link
Copy Markdown
Member

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

codecov Bot commented Apr 27, 2021

Copy link
Copy Markdown

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.

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