Wakoma / nimble

The nimble. An open source, rapidly deployable, wireless mesh network.
CERN Open Hardware Licence Version 2 - Strongly Reciprocal
62 stars 10 forks source link

Further standardise nimble parameters #58

Open julianstirling opened 4 months ago

julianstirling commented 4 months ago

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

jmwright commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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:

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 commented 3 months ago

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 commented 3 months ago

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