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

Updates stephan #268

Merged
merged 80 commits into from
Sep 25, 2023
Merged

Updates stephan #268

merged 80 commits into from
Sep 25, 2023

Conversation

stschiff
Copy link
Member

I am working on a few updates and some refactoring of #264

…y structure, which also supports versioned packages
@nevrome
Copy link
Member

nevrome commented Sep 20, 2023

Excellent - some last comments (I hope):

  • I think list should have an extra, boolean column for the latest property. A (latest) suffix to the version number is harder to parse, especially for quick analyses on the command line.
  • A number of subcommands (genoconvert, rectify, survey, summarize, validate) now only operate on the latest package versions in a given base directory. This may be confusing -- especially if there is no clear need to only consider one iteration of a package (e.g. in genoconvert, survey or validate). Should we make them multi-version compatible? Maybe they should behave like list: Always consider all package versions, except the --onlyLatest flag is set. This doesn't fully match what users might expect for rectify and summarize, but it would be a universal and consistent solution. We could essentially set _readOptKeepMultipleVersions in defaultPackageReadOptions to true.
  • The technical, internal changelog should probably also mention the high-level refactoring within the library: The SecondaryTypes module was dissolved and instead we now have Contributor, ServerClient and Version. EntitiesList (or what is left of it) now lives in EntityTypes.
  • The way we handle versions now (e.g. in parseVersion) is a bit imprecise. According to the implementation we only accept versions of the form X.Y.Z, but the Poseidon v2.7.1 standard does not specifically say this. I think we should make this constraint explicit in the next version of the standard.

Let me know if you agree with these ideas and whether I should take over and implement some of them.

@stschiff
Copy link
Member Author

OK, list now has its own column for "Is latest". I also noticed that the raw output so far did not print out the column headings, which is however vital, in particular with additional janno columns when listing individuals, so I now added this to the raw output (see for example golden test output for test-pipeline list4.

@stschiff
Copy link
Member Author

Regarding the second point, I have now changed the default behaviour. I looked at the golden test outputs, and think that summarise looks strange when applied to multiple versions. I think it makes no sense in that specific case, so I explicitly now set _readOptKeepMultipleVersions = False in summarise. Happy to discuss

@stschiff
Copy link
Member Author

Regarding points 3 and 4 I agree! I think you wanted to take a stab at the release changelog, and regarding point 4 yes, I will make a PR to the schema (no Poseidon version change, just a clarification, I think)

src/Poseidon/Package.hs Outdated Show resolved Hide resolved
@stschiff
Copy link
Member Author

OK very cool, I've reviewed the code changes and ran a few command line tests on the test-data. I'm happy. What do you say? I say, we merge and start a test-release, then get the rest of the ball going. Of course, finalist the release only after further tests and update of the server.

@stschiff
Copy link
Member Author

I've added a fix for validate, which would fail if some packages were skipped if --onlyLatest is selected.

@stschiff stschiff merged commit 473325d into master Sep 25, 2023
4 checks passed
@stschiff stschiff deleted the updatesStephan branch September 25, 2023 09:29
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.

None yet

2 participants