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

[INFRA] Optional tdl #218

Merged
merged 1 commit into from
Jan 31, 2024
Merged

[INFRA] Optional tdl #218

merged 1 commit into from
Jan 31, 2024

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Jan 30, 2024

Tests are a bit annoying, and snippets will need to be built with tdl (readme_snippet).

Copy link

vercel bot commented Jan 30, 2024

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

Name Status Preview Updated (UTC)
sharg-parser ✅ Ready (Inspect) Visit Preview Jan 31, 2024 0:59am

@seqan-actions seqan-actions added the lint [INTERNAL] signals that clang-format ran label Jan 30, 2024
@eseiler eseiler force-pushed the infra/optional_tdl branch from 9220806 to 641a878 Compare January 30, 2024 19:29
@seqan-actions seqan-actions removed the lint [INTERNAL] signals that clang-format ran label Jan 30, 2024
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (60abdbe) 91.90% compared to head (82a96f7) 91.91%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #218   +/-   ##
=======================================
  Coverage   91.90%   91.91%           
=======================================
  Files          17       17           
  Lines        1594     1595    +1     
=======================================
+ Hits         1465     1466    +1     
  Misses        129      129           

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

@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Jan 30, 2024
@eseiler eseiler force-pushed the infra/optional_tdl branch from 9ccc8ec to 7932c77 Compare January 30, 2024 23:39
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Jan 30, 2024
@eseiler eseiler marked this pull request as ready for review January 31, 2024 11:25
Copy link
Contributor

@SGSSGene SGSSGene left a comment

Choose a reason for hiding this comment

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

Thank you!

include/sharg/detail/to_string.hpp Show resolved Hide resolved
Copy link
Member Author

@eseiler eseiler left a comment

Choose a reason for hiding this comment

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

💅

include/sharg/detail/format_tdl.hpp Outdated Show resolved Hide resolved
test/unit/detail/CMakeLists.txt Outdated Show resolved Hide resolved
@eseiler eseiler force-pushed the infra/optional_tdl branch from fae9384 to 82a96f7 Compare January 31, 2024 12:58
@seqan-actions seqan-actions added lint [INTERNAL] signals that clang-format ran and removed lint [INTERNAL] signals that clang-format ran labels Jan 31, 2024
@eseiler eseiler enabled auto-merge January 31, 2024 13:27
@eseiler eseiler merged commit 518c1bb into seqan:main Jan 31, 2024
29 checks passed
@eseiler eseiler deleted the infra/optional_tdl branch January 31, 2024 13:37
@h-2
Copy link
Member

h-2 commented Feb 2, 2024

Thank you for the quick fix again!

One thing though: right now you are assuming that the library is always built by CMake, because you are referring to SHARG_HAS_TDL but this has no default value in the code. In practice, this works for me right now, because #if SHARG_HAS_TDL evaluates to false if SHARG_HAS_TDL is not defined. But a better solution would IMHO be to set a default in the source code. Either =0 or based on the presence of TDL headers, e.g.:

#ifndef SHARG_HAS_TDL
#define SHARG_HAS_TDL __has_include(<TDLHEADER>)
#endif

That way, everything would still work well in header-only land.

@eseiler
Copy link
Member Author

eseiler commented Feb 2, 2024

Thank you for the quick fix again!

One thing though: right now you are assuming that the library is always built by CMake, because you are referring to SHARG_HAS_TDL but this has no default value in the code. In practice, this works for me right now, because #if SHARG_HAS_TDL evaluates to false if SHARG_HAS_TDL is not defined. But a better solution would IMHO be to set a default in the source code. Either =0 or based on the presence of TDL headers, e.g.:

#ifndef SHARG_HAS_TDL
#define SHARG_HAS_TDL __has_include(<TDLHEADER>)
#endif

That way, everything would still work well in header-only land.

I was thinking about adding that in the platform.hpp, but then didn't do it, because it'll evaluate to false anyway :D
Maybe I'll add that to be more correct.

The header-check would cause issues if TDL is installed system-wide.
Then the <tdl/tdl.h> would be available, but you wouldn't link against it.

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.

4 participants