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 or remove settings equality operators #1001

Closed
confluence opened this issue Jan 13, 2022 · 2 comments · Fixed by #1329
Closed

Fix or remove settings equality operators #1001

confluence opened this issue Jan 13, 2022 · 2 comments · Fixed by #1329
Assignees
Labels
code maintenance For issues relating to code maintenance and quality
Milestone

Comments

@confluence
Copy link
Collaborator

The equality operators for ProgramSettings use a GetTuple function which hasn't been updated to include new fields (so they no longer actually check for equality). The equality operator is only used in the empty arguments test of the settings class. Adding more of the fields causes the test to fail, because e.g. event_thread_count is set to a new value dynamically when CARTA is run with no parameters.

Either the test needs to be revised to work with an operator which correctly checks for equality, or the operator should be removed from the main codebase and replaced with a test function which is more specifically tailored to the test.

I will commit a proof of concept updated equality operator to a branch.

@confluence
Copy link
Collaborator Author

A proof-of-concept solution for updating the operators is in the branch 1001_settings_equality_operators.

  • This solution does not check additional fields like no_user_config, etc., which should possibly also be added for completeness.
  • There's a typo in the test name which this branch fixes.

Given that the operator is only used in this test, I think it would probably be a better idea to remove the operator and rewrite the test.

@kswang1029 kswang1029 added the code maintenance For issues relating to code maintenance and quality label May 9, 2023
@confluence confluence self-assigned this Sep 26, 2023
@confluence
Copy link
Collaborator Author

I would suggest removing the operator entirely and rewriting the test to add some kind of test-specific comparison function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code maintenance For issues relating to code maintenance and quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants