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

Fix #152 - Bump version, documentation, tidy up #162

Merged
merged 22 commits into from
May 16, 2024

Conversation

lars-t-hansen
Copy link
Collaborator

Evolving - to land after everything else has landed.

@bast
Copy link
Member

bast commented Apr 4, 2024

After I merge and resolve the other PRs, I will fix conflict here and also update the lock file and add it to this PR.

@bast
Copy link
Member

bast commented Apr 4, 2024

Conflict fixed and Cargo.lock updated.

@bast
Copy link
Member

bast commented Apr 5, 2024

Before merging, let's run cargo clippy. It indicates few potential problems not caught by cargo check.

@lars-t-hansen
Copy link
Collaborator Author

Formatting might be in order, too.

@lars-t-hansen
Copy link
Collaborator Author

I'll pull it over to a machine with GPUs to make sure things look OK there too.

@lars-t-hansen
Copy link
Collaborator Author

I introduced a bug in command line parsing when I removed clap. Previously for sonar one could write --option=value not just --option value. The scripts use the former syntax and I think it's useful to allow that. I'll create a fix. To keep things simple, I'll attach the fix to this PR.

@lars-t-hansen
Copy link
Collaborator Author

That seems to work fine. I'm going to think about whether we can have some test cases to test command line parsing.

@lars-t-hansen
Copy link
Collaborator Author

I guess I went a little crazy with the testing. But it's time to review this probably.

@lars-t-hansen lars-t-hansen marked this pull request as ready for review April 8, 2024 09:21
@lars-t-hansen lars-t-hansen requested a review from bast April 8, 2024 09:21
Cargo.toml Show resolved Hide resolved
@bast
Copy link
Member

bast commented Apr 8, 2024

Are the tests run as part of the workflow/pipeline/actions?

@lars-t-hansen
Copy link
Collaborator Author

Are the tests run as part of the workflow/pipeline/actions?

They are not, for two reasons (that are mostly the same reason). One, I don't know how to make that happen :-) Two, one of the tests has a dependency on an external tool jq to syntax check JSON, and I don't know how to set that up.

The dependency on jq can probably be fixed, if it turns out to be a problem. It was an expedient solution. We could create simple, custom testing programs for syntax checking JSON and CSV, and use those here. Indeed the Jobanalyzer repo already has one of those that I wrote before discovering jq.

It would be great if these tests could be run as part of the workflow, though.

@bast
Copy link
Member

bast commented Apr 10, 2024

OK I will try to add those to our workflow.

@lars-t-hansen
Copy link
Collaborator Author

Rebased.

@bast
Copy link
Member

bast commented Apr 24, 2024

Working on adding the tests ...

@lars-t-hansen
Copy link
Collaborator Author

Working on adding the tests ...

That's great. When you get to a point where it works, also note some tests landed with #169 that should be added to the test runner.

@bast
Copy link
Member

bast commented Apr 24, 2024

Rebasing ...

@bast
Copy link
Member

bast commented Apr 24, 2024

The test fails now but for good reasons.

@bast
Copy link
Member

bast commented May 16, 2024

Can you please verify 284d507? This restores one shell test but I want a second pair of eyes that I did not misunderstand something.

@lars-t-hansen
Copy link
Collaborator Author

Yes, that looks right - --bartchless and --rollup are incompatible now, so splitting the test is the best fix.

@bast
Copy link
Member

bast commented May 16, 2024

Now something else fails. But it passes on my laptop. Looking into it ...

@bast bast force-pushed the w-152-final-cleanup branch 2 times, most recently from 7065077 to bda9d8f Compare May 16, 2024 10:52
@bast bast force-pushed the w-152-final-cleanup branch 2 times, most recently from ec23111 to 09d2b0b Compare May 16, 2024 11:01
@bast
Copy link
Member

bast commented May 16, 2024

Now we are all green finally. Ready to go from my perspective.

@lars-t-hansen
Copy link
Collaborator Author

I'll take a quick look.

@lars-t-hansen
Copy link
Collaborator Author

LGTM, merge at will. I don't think I had anything outstanding on this one, getting rid of subprocess would be nice but I think that's a separate Issue.

@bast bast merged commit 4a41c81 into NordicHPC:main May 16, 2024
1 check passed
@lars-t-hansen lars-t-hansen deleted the w-152-final-cleanup branch May 16, 2024 11:28
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