Skip to content

patch: using newest TDL#211

Merged
eseiler merged 2 commits intoseqan:mainfrom
SGSSGene:patch/update_tdl
Oct 7, 2023
Merged

patch: using newest TDL#211
eseiler merged 2 commits intoseqan:mainfrom
SGSSGene:patch/update_tdl

Conversation

@SGSSGene
Copy link
Copy Markdown
Contributor

@SGSSGene SGSSGene commented Oct 5, 2023

Update sharg to newest TDL

@vercel
Copy link
Copy Markdown

vercel bot commented Oct 5, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sharg-parser ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 7, 2023 0:26am

@SGSSGene SGSSGene mentioned this pull request Oct 5, 2023
Closed
12 tasks
@seqan-actions seqan-actions added the lint [INTERNAL] signals that clang-format ran label Oct 5, 2023
@SGSSGene SGSSGene marked this pull request as draft October 5, 2023 08:06
@seqan-actions seqan-actions removed the lint [INTERNAL] signals that clang-format ran label Oct 5, 2023
@SGSSGene SGSSGene marked this pull request as ready for review October 5, 2023 11:22
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Oct 5, 2023
@SGSSGene SGSSGene requested review from eseiler and smehringer October 5, 2023 11:23
@SGSSGene SGSSGene marked this pull request as draft October 5, 2023 11:42
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Oct 5, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 5, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (a1a5f96) 91.77% compared to head (6b54ce2) 91.78%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #211   +/-   ##
=======================================
  Coverage   91.77%   91.78%           
=======================================
  Files          17       17           
  Lines        1593     1594    +1     
=======================================
+ Hits         1462     1463    +1     
  Misses        131      131
Files Coverage Δ
include/sharg/detail/format_tdl.hpp 94.01% <100.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

smehringer
smehringer reviewed Oct 5, 2023
View reviewed changes
include/sharg/detail/format_tdl.hpp
parameters = {tdl::Node{
.name = executable_name[executable_name.size() - i],
.tags = {"basecommand"},
.value = parameters,
Copy link
Copy Markdown
Member

@smehringer smehringer Oct 5, 2023

Choose a reason for hiding this comment

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

The value is again the parameters that is just filled?

Copy link
Copy Markdown
Contributor Author

@SGSSGene SGSSGene Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

good question!
I changed slightly what TDL is expecting.
Imagine we have a call like raptor build --input somefile.fa --kmer 5, then --input and --kmer are subparameters of build. This is now better reflected in TDL.
But I don't know if someone is calling raptor ..., or raptor build or something like git remote add ....
So I fill parameters with --input and --kmer and than later nest it into a build section. (or if there are more subcommands, multiple iterations).

The basecommand is a special tag for TDL/CWL expressing that the name has to be part of the command line.

include/sharg/detail/format_tdl.hpp
info.cliMapping.emplace_back("", name);
parameters = {tdl::Node{
.name = executable_name[executable_name.size() - i],
.tags = {"basecommand"},
Copy link
Copy Markdown
Member

@smehringer smehringer Oct 5, 2023

Choose a reason for hiding this comment

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

I have no real clue what's going on. But if the tag here is basecommand and a value is given, shouldn't at least one test have a value at the basecommand output below?

Copy link
Copy Markdown
Contributor Author

@SGSSGene SGSSGene Oct 5, 2023

Choose a reason for hiding this comment

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

basecommand is a special tag (we have other special tags, like output or directory). It marks that the name of this node/section has to be attached to the command line.

@SGSSGene SGSSGene marked this pull request as ready for review October 5, 2023 19:47
@SGSSGene SGSSGene marked this pull request as draft October 6, 2023 07:31
@SGSSGene
Copy link
Copy Markdown
Contributor Author

SGSSGene commented Oct 6, 2023 •
edited
Loading

I will keep this on hold, since there some fixes here:

@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Oct 6, 2023
@SGSSGene SGSSGene marked this pull request as ready for review October 6, 2023 13:04
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Oct 6, 2023
@SGSSGene
Copy link
Copy Markdown
Contributor Author

SGSSGene commented Oct 6, 2023 •
edited
Loading

On hold until this is fixed:

@SGSSGene SGSSGene marked this pull request as draft October 6, 2023 13:39
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Oct 7, 2023
@SGSSGene SGSSGene marked this pull request as ready for review October 7, 2023 12:26
@eseiler eseiler merged commit 492f148 into seqan:main Oct 7, 2023
@SGSSGene SGSSGene deleted the patch/update_tdl branch October 9, 2023 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smehringer smehringer smehringer approved these changes

@eseiler eseiler eseiler approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SGSSGene @smehringer @eseiler @seqan-actions