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

Lower case args for CadToCsg class #118

Closed
wants to merge 3 commits into from

Conversation

shimwell
Copy link
Collaborator

@shimwell shimwell commented May 1, 2024

This PR changes the args to the main CadToCsg class so that they are lower case.

These changes are user facing so will causes some disruption for people using the dev branch.

Sorry for these snake case PRs I realise they are breaking changes.

I can make some docs to help facilitate these changes

@psauvan
Copy link
Member

psauvan commented May 1, 2024

Changing the parameters name the geouned user will see, is something I am not confortable with.
Personally I don't like snake case, but if it is a standard for programmers I can accept changing the internal variable names in the code.
But for user interface why cannot we keep the current variable name?

@shimwell
Copy link
Collaborator Author

shimwell commented May 1, 2024

We can certainly keep the current names if you prefer. Like all my PRs this is just an idea for your consideration.

The reason for this PR is that python convention and PEP8 naming recommendations that these attributes (like function names, arguments and variables) are in snake_case.

One of the reasons for python having naming conventions is to make the code predictable and therefore easier for the python user. Currently the user has to guess at the argument naming as some are:

  • some are camel case (stepFile, geometryName, matFile , compSolids, dummyMat, startCell , startSurf, minVoidSize, voidMat, cellRange, cellSummaryFile, cellCommentFile, exportSolids
  • some are upper case for multiple letters (volSDEF, volCARD)
  • some are fully upper case (UCARD)

Other programming languages choose other naming conventions and I personally really like the kebab-case that lisp opted for.

However as this is a python package there is a benefit of trying to conform to the language norms.

Before fully considering this PR let me show you the docs I've been working on as I think they will help users know the argument names and minimise the disturbance.

@shimwell
Copy link
Collaborator Author

shimwell commented May 1, 2024

I've got some docs online to show one way this change can be communicated to users. This is build automatically from the docstrings and type hints

https://shimwell.github.io/GEOUNED/python_api.html

I think something like this would be helpful as it shows the changes to the user facing code.

Also another way this change is communicated to the user is that anyone with an IDE will also get the auto complete thanks to the doc strings on the class

Screenshot 2024-05-01 175835

And also hover text with the argument names
Screenshot 2024-05-01 175754

@psauvan
Copy link
Member

psauvan commented May 2, 2024

Lets think a little more on it.
May be we can rethink the name of some user parameters to give them some "standardization", but at the end I would prefer to not change the user name to snake case.

@shimwell
Copy link
Collaborator Author

closing as this has fallen quite far behind the dev branch and it is easier to make a new PR than resolve the conflicts for this one

@shimwell shimwell closed this May 10, 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