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

Features/#355 module use support #403

Merged
merged 16 commits into from
Aug 5, 2021
Merged

Conversation

dakochik
Copy link
Contributor

'module' and 'use' sections highlighting is done

Resolved: #355

@dakochik dakochik assigned dakochik and iromeo and unassigned dakochik Jul 27, 2021
@iromeo iromeo self-requested a review July 27, 2021 16:48
Copy link
Contributor

@iromeo iromeo left a comment

Choose a reason for hiding this comment

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

In general is good, some case not supported and tests missing. Better to split such huge features (parsing, completion, resolve, inspections) in subtasks and do them sequentially or at least in different commits. One commit should deliver a single idea, not many complex features at once otheriwise it is hard to do code review, check tests coverage, etc.

@iromeo iromeo assigned dakochik and unassigned iromeo Jul 27, 2021
Dmitry Kochik and others added 5 commits July 28, 2021 16:21
@dakochik
Copy link
Contributor Author

fixed:

  • parsing logic in commit 479c04a, which affects errors messages, acceptable redeclared rule names, separate tokens for snakemake 'as', 'with' and 'from keywords', references and arguments section parsing. Also this commit adds tests, that check parsing errors.
  • commit 8b62434 affects code completion in declaration of use section and adds tests, that checks completion and annotator highlighting.

Answers to some questions:

  1. What if use rule * from rule_355_module as rule_foo ? Is it runtime error?

I've checked and found, that snakemake would use as rule_foo only the last rule in imported module rule_355_module

  1. is text_*_text_*_text also allowed on runtime?

Yes :/
Commit 479c04a adds support for such names

@dakochik dakochik assigned iromeo and unassigned dakochik Jul 28, 2021
Copy link
Contributor

@iromeo iromeo left a comment

Choose a reason for hiding this comment

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

  • Not all use syntax support. Maybe is good to look through python impl of use/module parsing in snakemake sources if documentation doesn't cover all cases.
  • Also let's make syntax errors look like snakemake runtime error messages for user convenience (although some of our messages IMHO are better, so let's took best from snakemake runtime errors and our impl).
  • List of forbidden sections for use seems to be different. Here better is to check snakemake python impl details. E.g. 'benchmark' also is not allowe in use, not only execution sections. Also wrong section name result in weird runtime parsing error, so likely list of supported sections is harcoded in snakemake parser class

@iromeo iromeo assigned dakochik and unassigned iromeo Jul 29, 2021
Dmitry Kochik added 4 commits July 30, 2021 10:21
…harm into features/#355-module-use-support

� Conflicts:
�	CHANGELOG.md
…ion, comma-separated list of imported rules names), additional tests, errors messages are were refactored, highlighting of Snakemake 'from', 'as' and 'with' keywords

Resolves: #355
… parsing comments and function calls

Resolves: #355
@dakochik
Copy link
Contributor Author

dakochik commented Aug 3, 2021

fixed:

  • Support for use section declaration with list of imported names see 0c79a6f
  • Support for use section with single-line declaration see 0c79a6f
  • Error messageas are changed and new messages are added see 0c79a6f
  • Now parser tries to parse use section arguments sections even if there no with: see 0c79a6f
  • Parser error highlighting tests are changed from .feature to .smk see 0c79a6f
  • with completion for (see 5f6371c):
use rule rule from other_355_workflow : 
    input: "rule_355.input"
  • Highlighting as, from, with by smk annotator see 0c79a6f
  • Code refactoring in SmkRuleOrCheckpointNameReference see 0c79a6f

@dakochik dakochik assigned iromeo and unassigned dakochik Aug 3, 2021
Copy link
Contributor

@iromeo iromeo left a comment

Choose a reason for hiding this comment

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

Almost done =)

@iromeo iromeo assigned dakochik and unassigned iromeo Aug 3, 2021
@dakochik
Copy link
Contributor Author

dakochik commented Aug 4, 2021

fixed: errors messsages, parsing tests
refatored: parser code

@dakochik dakochik assigned iromeo and unassigned dakochik Aug 4, 2021
@iromeo iromeo merged commit 82271fa into master Aug 5, 2021
@iromeo iromeo deleted the features/#355-module-use-support branch August 5, 2021 13:55
iromeo added a commit that referenced this pull request Oct 7, 2024
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.

Syntax support for Snakemake 6.x 'module' and 'use' concepts
2 participants