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

--preservePyml #304

Merged
merged 9 commits into from
Jul 11, 2024
Merged

--preservePyml #304

merged 9 commits into from
Jul 11, 2024

Conversation

nevrome
Copy link
Member

@nevrome nevrome commented Jun 28, 2024

To conclude the --ordered saga I drafted an implementation for the --preservePyml argument as discussed in #300. I had to make some design decisions you may or may not like, @stschiff and @TCLamnidis, so please comment.

I went for a simple --preservePyml switch that only works in case there is a single relevant source package. If there are more, and the option is active, then forge stops with an error.
With --preservePyml forge runs through normally, but then replaces the following fields of the generated POSEIDON.yml file from the source POSEIDON.yml file:

  • description
  • contributor
  • packageVersion
  • lastModified
  • readmeFile
  • changelogFile

Copying the readmeFile and changelogFile fields requires of course also to copy the respective files.

You'll note that the package title is not copied. It can be easily reproduced by the user with the -o or -n options.

There is a bit of a weird interaction with the --minimal flag. With --preservePyml and --minimal the resulting package may have a README and a CHANGELOG file, but not a .janno or a .bib file. This sounds like one of these unexpected interactions that could bite us eventually...

I haven't written any tests yet.

Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 77.31959% with 22 lines in your changes missing coverage. Please review.

Project coverage is 68.50%. Comparing base (a141fc3) to head (14baad7).

Files Patch % Lines
src/Poseidon/CLI/Forge.hs 78.12% 4 Missing and 17 partials ⚠️
src/Poseidon/Utils.hs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   68.21%   68.50%   +0.28%     
==========================================
  Files          26       26              
  Lines        3492     3540      +48     
  Branches      394      399       +5     
==========================================
+ Hits         2382     2425      +43     
  Misses        716      716              
- Partials      394      399       +5     

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

@nevrome nevrome requested a review from stschiff June 28, 2024 14:35
@stschiff
Copy link
Member

stschiff commented Jul 2, 2024

Thanks, will take a look.

Copy link
Member

@stschiff stschiff left a comment

Choose a reason for hiding this comment

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

OK, I checked the code, without trying it out, and I can green light all of these changes. Looks good to me. Would be nice to turn at least one of the existing forge-golden-tests into a test for --preservePyml. Would be easy enough I think.

Regarding the --minimal flag, I must admit I never fully understood what use-case that actually supported. I understand why it's in init, but for forge I never got the point. In any case if we are serious about it, we should not allow --minimal and --preservePyml together, simple as that: With --minimal a user has expectations for empty fields in the Yaml file, while with --preservePyml those very fields are filled. These two expectations are fundamentally incompatible with each other. So I would suggest not to allow that.

@nevrome
Copy link
Member Author

nevrome commented Jul 11, 2024

I used --minimal in the past for pipelines, where I wanted to keep the intermediate data consistently in packages. This was before --onlyGeno, though, so maybe it's less relevant now. Still convenient to have it, so I don't want to touch it at the moment.

Anyway: I refactored forge to replace the increasingly complex boolean logic by a clear separation of output modes with the new type ForgeOutMode. We now have four mutually exclusive output options: --onlyGeno, --minimal, --preservePyml and the default (all covered by golden tests). There are no more unpredictable interactions between them. I think it's much more clear like this.

@nevrome nevrome merged commit e8ff841 into master Jul 11, 2024
4 checks passed
@nevrome nevrome deleted the preservePyml branch July 11, 2024 13:24
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.

2 participants