feat: update to black v24 and fix double curly braces #216
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #216 +/- ##
==========================================
+ Coverage 97.76% 97.82% +0.06%
==========================================
Files 12 12
Lines 1028 1057 +29
Branches 228 237 +9
==========================================
+ Hits 1005 1034 +29
Misses 12 12
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Merge and new release? |
I'd rather wait for @bricoletc to review if that's okay? He often will find things I've missed or suggest better ways to do certain things (he is more familiar with the parser logic than me). |
|
@mbhall88 @johanneskoester I will review this by the end of the week ideally 👍 |
|
Sure! |
There was a problem hiding this comment.
V. nice commits @mbhall88, clearly #215 is fixed
In the future I think we should PR from a non-master branch on snakefmt itself; that way, when one of us reviews, we can add commits to that branch, which updates the PR here on github
I've added two commits to my fork that I would like to put on top of mbhall/master (i.e. this PR) - do you know what's the best way to do this?
And I've left comments
Thanks :))
| DEFAULT_TARGET_VERSIONS = { | ||
| TargetVersion.PY38, | ||
| TargetVersion.PY39, | ||
| TargetVersion.PY310, | ||
| TargetVersion.PY311, | ||
| TargetVersion.PY312, | ||
| } |
There was a problem hiding this comment.
Just checking I'm following here @mbhall88, what does this achieve exactly? I'm guessing Black's Mode constructor now requires the target_version argument, but does the set of target versions provided change how Black will format the code??
There was a problem hiding this comment.
This also forces up to maintain in two places what python versions we support - the 'classical' pyproject.toml, and here.
There was a problem hiding this comment.
It was more to be explicit with black that we are targeting these versions as I suspect there might be python >=3.12-specific formatting changes that can happen with the new f-string functionality.
I know it's a pain to maintain in two places....Maybe we can make this just target the version of python that is being used when snakefmt is run...
There was a problem hiding this comment.
Yes that would be better IMO - do you want to try and write that up, or do you prefer that I do it?
There was a problem hiding this comment.
Happy for you to do it. Should be as easy as using sys to check the current version of python. Though, I wonder if black actually already does this?
|
Erm, @mbhall88 sorry I made a mistake, I mistakenly merged this into master here, didn't mean to. I've reversed master here back to v0.9.0 . |
This PR closes #215, which was caused by the different f-string tokenizing in python 3.12. We had previously dealt with this (#213), but this was only in snakemake code. #215 relates to python code outside of snakemake code (i.e. outside of rules).
The PR also updates black to v24.1, which contains some style changes we need to make some alterations for:
"""module string"""). We had some test cases where we allow multiple module strings to be passed to rule directives such asshellandmessage, so I had to add some code to remove black's added newline in these cases.if-elsestatement is now broken into three linesI also explicitly added a default for the python versions we target to black.