Root endpoint with Wifi support; Wifi NW diagnostics cluster #169
Conversation
| /// The type of operational network (Ethernet, Wifi or (future) Thread) | ||
| /// for which root endpoint meta-data is being requested | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | ||
| pub enum OperNwType { |
There was a problem hiding this comment.
Is there precedent on naming? In other projects I was generally advised by styling guides to optimize readability even if typing speed slows down (and autocomplete helps anyway). Should we consider naming this OperationalNetworkType to fully expand things?
There was a problem hiding this comment.
I can change it in the blink of an eye, but then... I'm not so sure it is back or white:
Spelling things out is a hard Java rule (from where I'm coming from); status quo in Rust seems a bit different, in that abbreviations are seen quite more often - from keywords to type names.
I also tried to match the naming rules "around" the new name I introduce. For better or worse, the code around that type contained "Nw" (not "Network"). And "Eth". And quite a few other abbreviations. The initial name btw was OperationalNwType but it looks silly to abbreviate the shorter word (network), and keep "operational" spelled out. So I shortened them both.
If that is not convincing - I can still change it though. Just wanted to show a bit that I had similar doubts, even if I decided completely the opposite of what your doubts are hinting you. :)
There was a problem hiding this comment.
The Nw is what tripped me up ... kept reading it as North-west.
Java is overly verbose, but also https://google.github.io/styleguide/cppguide.html#General_Naming_Rules says to not try to save horizontal space.
Not a PR blocker though ... if Nw is used in other places (I did not know that) it seems consistency is reasonable. Will approve things either way - feel free to change or close this thread as well.
There was a problem hiding this comment.
Outside of the scope of this PR (due to seemingly mass renames), https://www.abbreviations.com/abbreviation/Network seems to say that we should use Net or Ntwrk or NetWk ... seems heavily biased to Net
https://www.allacronyms.com/network/abbreviated has NW appearing, but also heavily biased on Net
|
I've now opened this as I feel that rather than fixing stuff "on the spot" (and thus maybe actually introducing even more inconsistencies), we should have an ongoing task to fix our abbreviations throughout the code base. |
(A follow up on #167 , as promised there.)
The existing types/struct which were actually hard-coded for Ethernet are now type-aliased / delegate to the new types and are renamed as follows:
root_endpont::CLUSTERS->root_endpoint::ETH_CLUSTERS(and there's alsoWIFI_CLUSTERSnow)root_endpoint::endpointtakes an extra parameter -OperNwTypeRootEndpointHandlertype ->EthRootEndpointHandlertype, and there is a newly-introducedRootEndpointHandlerwhich is generified byNWCOMMandNWDIAGroot_endpoint::handler->root_endpoint::eth_handler. Here also, newly introduced more genericroot_endpoint::handlerfn which takes extra parameters... Looking at the "on off" example actually shows the API-visible changes. Minor breakage to backward compatibility - users just need to prefix a few things with
eth_now.