Skip to content

Fix YAML input for top_traj_file: #75

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.

Sign up for GitHub

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

Jump to bottom
Merged
harryswift01 merged 2 commits into from
Mar 25, 2025
Merged

Fix YAML input for top_traj_file: #75

harryswift01 merged 2 commits into from
Mar 25, 2025

Conversation

@harryswift01
Copy link
Contributor

@harryswift01 harryswift01 commented Mar 25, 2025

Summary

This PR updates the argument handling for top_traj_file to remove its default value, ensuring proper merging of YAML and CLI inputs.

Changes

top_traj_file argument handling:

  • Removed the default value for top_traj_file, which previously overrode the merging of YAML and CLI inputs.
  • Updated tests to reflect this change and ensure correct argument parsing.

Impact

  • This will allow the top_traj_file argument to be correctly merged from both YAML and CLI inputs without being overridden by a default value.
  • Improves the flexibility and accuracy of argument handling in the application.

@harryswift01
Fix YAML input for top_traj_file:
37a568c 
- top_traj_file had a default value which meant it overided the merging of YAML and CLI
- Removed the default value
- Fixed tests to account for this change
@harryswift01 harryswift01 added the bug Something isn't working label Mar 25, 2025
@harryswift01 harryswift01 added this to the WP7 - Refactor milestone Mar 25, 2025
@harryswift01 harryswift01 requested a review from jimboid March 25, 2025 12:16
@harryswift01 harryswift01 self-assigned this Mar 25, 2025
@harryswift01 harryswift01 linked an issue Mar 25, 2025 that may be closed by this pull request
YAML input not accepting top_traj_file input #74
Closed
@skfegan
Copy link
Member

skfegan commented Mar 25, 2025

@harryswift01 Have you added a check that gives a warning if anyone tries to add a new argument with a default of [] or "" or similar, so we don't get a repeat of this bug?

jimboid
jimboid approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@jimboid jimboid left a comment

Choose a reason for hiding this comment

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

Looks good. Test is present to check for badly defined defaults to avoid the odd pythonic behaviour.

@harryswift01 harryswift01 merged commit 7efbbc3 into main Mar 25, 2025
6 checks passed
@harryswift01 harryswift01 deleted the 74-yaml-input-bug branch March 25, 2025 15:40
@jimboid jimboid modified the milestones: WP7 - Refactor, 1.0.0 release Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimboid jimboid jimboid approved these changes

Assignees

@harryswift01 harryswift01

Labels

bug Something isn't working

Projects

None yet

Milestone

1.0.0 release

Development

Successfully merging this pull request may close these issues.

YAML input not accepting top_traj_file input

4 participants

@harryswift01 @skfegan @jimboid