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

Support new snakemake keywords prefix and default_target #131

Closed
wants to merge 4 commits into from

Conversation

koen-vg
Copy link
Contributor

@koen-vg koen-vg commented Jan 31, 2022

Hi! Thanks for the great project; snakefmt is much appreciated.

Recently two new keywords have been added to snakemake: prefix (in module blocks) and default_target (in rules). This pull request adds support to snakefmt for both. I have also included both in test. All tests pass, and I have also tested this with success on a more involved snakefile I personally work with.

This also closes #130.

Let me know if any improvements are changes are needed or wished for before merging.

The "prefix" keyword was introduced in snakemake 6.13. This commit
adds support for the keyword to snakefmt.
This keyword was added in snakemake 6.15.
@bricoletc bricoletc changed the base branch from master to dev January 31, 2022 20:15
@bricoletc
Copy link
Collaborator

bricoletc commented Jan 31, 2022

Hi @koen-vg , thanks for your contribution :) This looks good! By default we merge into dev and then push new versions to master.

I'm just a bit surprised that all tests passed for you, as our current snakemake dependency version fails 'grammar completeness' tests. So I recommend we merge this in dev and I then push an update of snakemake dep. version to latest (OR @koen-vg you push a commit on this branch after running poetry install(see CONTRIBUTING.md) and poetry update snakemake . I checked all tests/lints pass from there (make precommit)

@bricoletc
Copy link
Collaborator

@mbhall88 after your review we either wait for @koen-vg to push that snakemake update commit here, or we merge to dev and do it ourselves

@koen-vg
Copy link
Contributor Author

koen-vg commented Jan 31, 2022

Right, I did have to update to the latest version of snakemake, 6.15, but seem to have forgotten to document that. I'll try to push the snakemake update, that would be to the poetry.lock file?

This is needed since we support the `default_target` keyword
introduced in snakemake 6.15.
@koen-vg
Copy link
Contributor Author

koen-vg commented Jan 31, 2022

Alright I have to admit I'm not super familiar with poetry; the latest commit is what I got after updating the snakemake version in pyproject.toml and then running poetry update; hopefully that was the intention.

@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #131 (13614af) into dev (97a2b6e) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 13614af differs from pull request most recent head c9a00a4. Consider uploading reports for the commit c9a00a4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #131      +/-   ##
==========================================
- Coverage   98.21%   98.20%   -0.02%     
==========================================
  Files          12       12              
  Lines         954      948       -6     
  Branches      175      209      +34     
==========================================
- Hits          937      931       -6     
  Misses         10       10              
  Partials        7        7              
Flag Coverage Δ
unittests 98.10% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
snakefmt/parser/grammar.py 100.00% <ø> (ø)
snakefmt/parser/parser.py 96.39% <0.00%> (-0.04%) ⬇️
snakefmt/parser/syntax.py 99.05% <0.00%> (-0.01%) ⬇️
snakefmt/exceptions.py 100.00% <0.00%> (ø)

@mbhall88
Copy link
Member

mbhall88 commented Feb 1, 2022

All looks good. @koen-vg could you please add your changes to the CHANGELOG as well?

@bricoletc
Copy link
Collaborator

Thanks @koen-vg ! so I've just done two things:

  • Fixed CHANGELOG conflict in this PR on dev branch
  • Modified your poetry update commit, because you ran poetry update but I would rather we do poetry update snakemake only here (after, as you did, modifying the pyproject.toml snakemake version). I know poetry is not so easy at first (https://python-poetry.org/docs/basic-usage/#specifying-dependencies). For me, no need to update all dependencies, if they ain't broke, let's not change them :-) @mbhall88 thoughts?

@koen-vg I've pushed your changes plus those two points above to dev. You are still author of the fixing and changelog commits. @mbhall88 i would suggest we close this pull request since I've merged the material into dev, if you agree.

@koen-vg
Copy link
Contributor Author

koen-vg commented Feb 1, 2022

That all sounds very good. Thanks for making it easy to contribute to this project!

@mbhall88
Copy link
Member

mbhall88 commented Feb 1, 2022

For me, no need to update all dependencies, if they ain't broke, let's not change them :-) @mbhall88 thoughts?

Fine by me. This does remind me though, black is now stable so we will need to update that dependency soon. But agree with just updating snakemake here

@koen-vg I've pushed your changes plus those two points above to dev. You are still author of the fixing and changelog commits. @mbhall88 i would suggest we close this pull request since I've merged the material into dev, if you agree.

Ok. Normally I would prefer you push to this PR and we solve conflicts here, but it's done now. So yes, closing this makes sense.

@mbhall88 mbhall88 closed this Feb 1, 2022
@bricoletc
Copy link
Collaborator

bricoletc commented Feb 2, 2022

For future ref- not sure how I could have pushed a change that removed one of the commits (updating all dependencies), i.e. rewriting history, to the from branch?

If you have suggestions i'll do that next time round

@mbhall88
Copy link
Member

mbhall88 commented Feb 5, 2022

Ah didn't realise you had removed a commit. Ignore me.

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.

prefix keyword in module declaration not recognised
3 participants