Bugfixes and refactoring #35
Conversation
Unfortunately this means triple quoted strings get overindented for now
Could be dropped to the next parameter. With unit test.
With passing unit test.
* Config-related tests in their own source file * More grouping of tests under classes to document what they are testing in common
In test_config. testdir uses tmpdir fixture tmp directory as cwd. This avoids a bug in which subsequent unrelated tests were run in tmpdir due to os.chdir.
They are due to python formatting either: * Being triggered too early when exiting nested snakecode context * Being not triggered when entering nested snakecode context
And a grammar test passing formatting with all possibly duplicated keywords
* Config-related tests placed in test_config * Regex based snakefmt tests placed together and class renamed for clarity
bricoletc
requested review from
mbhall88
and removed request for
johanneskoester
Codecov Report
@@ Coverage Diff @@
## dev #35 +/- ##
==========================================
+ Coverage 94.09% 94.57% +0.48%
==========================================
Files 10 9 -1
Lines 711 738 +27
Branches 118 123 +5
==========================================
+ Hits 669 698 +29
+ Misses 31 29 -2
Partials 11 11
|
|
Ahh glad you found "Triple quoted strings can get over indented for eg in shell context". I was going to raise this. I think we should leave the (inside) of the shell command as is. i.e. only format that its start and end |
snakefmt/parser/syntax.py
Outdated
| } | ||
|
|
||
|
|
||
| def operator_skip_spacing(prev_token: Token, token: Token): |
snakefmt/parser/syntax.py
Outdated
| @@ -181,22 +195,26 @@ def effective_indent(self) -> int: | |||
|
|
|||
| def get_next_queriable(self, snakefile) -> Syntax.Status: | |||
snakefmt/parser/syntax.py
Outdated
| @@ -318,10 +344,12 @@ def flush_param(self, parameter: Parameter, skip_empty: bool = False) -> None: | |||
| if not parameter.has_value() and skip_empty: | |||
| return | |||
|
|
|||
| if parameter.has_key(): # noqa: W601 | |||
| if parameter.has_a_key(): # noqa: W601 | |||
There was a problem hiding this comment.
I guess we can remove the flake8 ignore comment now?
There was a problem hiding this comment.
yes, i changed the signature for that reason, will do!
|
Good pickup on the |
No longer indent triple quoted strings, they are left as is. With unit tests.
You're totally right. I think that only the first triple quote should conform to This inspired me and I implemented a fix, with two unit tests. |
|
The changes I've added have not led to the failing CI, i think it is tied to |
This PR closes #29 and #33 and refactors tests
Added
shellcontextblackedblacked but is an issueThe last two are totally related and should go in same issue. The first one doesn't really have to be an issue, not sure.
configfileas possibly duplicated keyword (Which keywords can occur more than once? #27), and a grammar test testing duplicated keywords pass.Changed
test_config.py. I find this much more modular and readable.os.chdir(tmpdir)when usingtmpdirpytest fixture. Unrelated tests can then run from inside there and were trying to use created pyproject.toml. So I switched to usingtestdirfixture which usestmpdirand starts the test in a tmpdir, but doesnt stay there. I added that plugin inconftest.py.CLIDiff,CLICheck, etc.. intest_snakefmt.