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

Further standardise nimble parameters #58

Open
7 tasks
julianstirling opened this issue May 16, 2024 · 5 comments
Open
7 tasks

Further standardise nimble parameters #58

julianstirling opened this issue May 16, 2024 · 5 comments

Comments

@julianstirling
Copy link
Collaborator

julianstirling commented May 16, 2024

The parameters in nimble_builder are either hard coded or repeatedly specified in different files. Now we have a RackParameters class we should be using this to define hole sizes.

We need to

  • Check through the code and find multiply defined parameters
  • Change the rack legs so that they are specified by height in units not in length
  • Make sure that screws go through clearance holes and then into tapped holes
  • Check that all screw holes sizes are defined from parameters and clearly specified as tapped or clearance, (or other such as counter sunk)
  • Decide on some standard rack parameters and think of how to pass specify them via cq-cli parameters
  • Be clear about which parameters are fixed and which can/should be changed. Improve the object and it's doc string to make this clear.
  • Stop using groups of strings for parameters and string comparisons, and start using enums
@jmwright
Copy link
Contributor

This might be scope creep for this PR, but we need to fix all the errant references to tray throughout the codebase. I think the "tray" naming convention came from me during the hackathon, but Wakoma uses the term "shelf", so we should fix the codebase to use that term. An example can be found here.

@julianstirling
Copy link
Collaborator Author

This issue is things that I want to do after PR #57 is merged. 57 is already huge, but I wanted a list of things to do afterwards so they were not forgotten.

I 100% agree that is it a shelf or a tray is a key question and we need to standardise. Will make new issue rather than scope creep this one

@drayde
Copy link
Contributor

drayde commented Jun 10, 2024

Concerning the task "Stop using groups of strings for parameters and string comparisons, and start using enums"

I'm actually a fan of parameter definitions like
front_type: Literal["full", "open", "w-pattern", "slots"]

My reasons:

  • Very concise definition, no need to define an extra class
  • Still a fully defined type
  • So type use can be automatically checked
  • Works with linters, and auto-completion in IDEs

If needed the types can also be named e.g.
AxisType = Literal["X", "Y", "Z"]

While there are still arguments to use enums in certain conditions, I would not vote for a general rule to ONLY use enums.

@julianstirling
Copy link
Collaborator Author

Thanks @drayde, I suppose I am somewhat biased by a combination of my terrible spelling and the tools that I use that don't autocomplete this well. I have done a bit more learning and I can see there is more to this that meets the eye.

In terms of linting, typed inputs. This is something I don't get from PyLint. I assume we need a type checker like MyPy?

I think that the thing I like least about them is not when you are making a call to a function with a Literal input, but when you are writing the function itself. For the function itself there is no type checking when you actually do the string comparison to use the parameters. A spelling error here would be a really hard bug to detect or lint. Which I suppose is an argument for unit tests.

I can see this comes down to personal preference.

@drayde
Copy link
Contributor

drayde commented Jun 13, 2024

You are right @julianstirling , flake8 or pylint won't catch a spelling mistake.
Did a quick test comparing the linters: https://colab.research.google.com/drive/1gXr8wK7oN7X13nq41VQ7BBuPwlnDOiiF
mytest reports it, pylint does not, flake8 only with the mypy extension

So I guess you are right, we should not use it unless we integrate mypy or do unit test that would catch it

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

No branches or pull requests

3 participants