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

passing specific args instead of class dict #122

Closed

Conversation

shimwell
Copy link
Collaborator

@shimwell shimwell commented May 2, 2024

Currently we pass a self.dict around to all the functions called.

This makes it difficult to know what functions need what data

knowing what functions need what data helps with a refactoring

while this is less concise than passing the whole class dictionary around it means we can see what is used where.

This PR is just a bare bones removal of the passing of the entire class dict

This makes it easier to create unit tests for individual function. Previously this would be difficult as one would have to make a complete CadToCsg class and pass that in as an argument

this large number of args passed from the class to the function suggests that the fucntion should be a class method instead. So we can follow this up with moving the function to the class, but first I'm keen to make all the information flow around the code clear

@shimwell
Copy link
Collaborator Author

shimwell commented May 2, 2024

Looks like all the tests pass but the new documentation one. This fails for the same reason as last time. A fork does not have permission to write to the gh-pages branch. I tweak that CI so that it runs post merge on a push event.

@shimwell
Copy link
Collaborator Author

shimwell commented May 2, 2024

Here is one more reason why we should pass the args down in this manner

It helps keep the code DRY as the args are more traceable as we can avoid remaking the same code block three times

For example this diff removes some duplicated code that was easy to spot with args
https://github.com/shimwell/GEOUNED/pull/6/files

@psauvan psauvan closed this May 3, 2024
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