Refactoring: shorten module names, remove some modules#248
Refactoring: shorten module names, remove some modules#248ivmarkov merged 20 commits intoproject-chip:mainfrom
Conversation
…s to types and make it private
…ssages into rs_matter::im
…mmon into rs_matter::sc
…vectors as a sub-module of spake2p
…ase38 codec that does not depend on the rest of rs_matter; rename and move utils/mod.rs to utils.rs
…e rs_matter::transport::core into rs_matter::transport
There was a problem hiding this comment.
Summary of Changes
Hello @ivmarkov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant internal refactoring of the rs-matter crate, focusing on simplifying module names and reorganizing the codebase for improved clarity and maintainability. The changes aim to make the project structure more intuitive by grouping related components under concise, top-level modules, without altering the external API behavior.
Highlights
- Module Renaming and Shortening: The primary refactoring involves shortening top-level module names:
data_modelis nowdm,secure_channelissc, andinteraction_modelisim. This change is applied consistently across the entire codebase, including examples, macros, and the core library. - Core Component Relocation: The
Matterstruct andMATTER_PORTconstant, previously residing inrs-matter/src/core.rs, have been moved directly intors-matter/src/lib.rs. Similarly, the main transport logic fromrs-matter/src/transport/core.rsis now inrs-matter/src/transport.rs. - Consolidated Functionality: Various related functionalities have been consolidated. For instance, Interaction Model (IM) and Secure Channel (SC) message definitions are now in dedicated
im.rsandsc.rsfiles respectively. Device attestation constants and vendor identifiers have also been moved to more central locations within thedmandpairingmodules. - Dependency Path Updates: All internal
usestatements and code references have been updated to reflect the new, shorter, and reorganized module paths. This includes paths to clusters, objects, devices, networks, and various protocol components.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refactoring module paths within the rs-matter crate, aiming to shorten module names and reorganize the structure. Key changes include renaming data_model to dm, interaction_model to im, secure_channel to sc, and moving several core components like Matter to the crate root. Most of the changes are consistent path updates in use statements and code.
I've identified two critical issues related to incorrect import paths for BasicInfoConfig in the mdns module, which will likely lead to compilation errors. The rest of the refactoring appears consistent.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
bjoernQ
left a comment
There was a problem hiding this comment.
Thanks for making this as reviewable as possible.
I'm not a big fan of two-letter abbrevs but I honestly don't have a reasonable alternative suggestion to them. Tried to look into what e.g. matter-js is doing but that didn't help really
In any case it's good to have these changes consolidated in one PR w/o any functional changes (in contrast to doing it whenever someone runs into a piece of code when implementing a functional change)
Well, the promise in #173 was that the big refactorings related to it would be just renames/moves. If you look at the downstream crates (PRs linked above), the only changed thing is a bunch of As for the |
|
@kedars Would be interested in your feedback as well. Will wait a few days before merging therefore. |
Yes, that's what I figured - I guess otherwise it's a fair assumption that anyone working on the code base is aware what these names mean |
This work is addressing module name issues described in #173.
While #233 and #237 did address the module names of the system clusters, and the system handlers' names themselves, these PRs did not touch the "more generic" module names, so to say, which this PR addresses.
The PR is very large as it is mostly automated work.
It cannot be reviewed line by line (obviously). Instead, I've tried to keep it as a series of separate commits, where each commit resulted in a type-checking, tests-passing outcome, and each commit is kind of self-describing as to what was the refactoring change.
Additionally, I provide a summary of the changes below:
mod.rsis gone from the codebasePreviously, the older modules used the older
mod.rspattern, while the newer ones used the newer style (i.e.foois infoo.rsand thenfoo/contains sub-modules only.Now everything except one single module in
tests/is using the newer pattern.coreis goneOlder
rs-mattermodules used a slightly less idiomatic pattern mentioned in #173, where bigger modules, like - say -transport- or even the root module - contained only sub-modules, while the "meat" of - say - thetransportmodule - was present intransport::coreinstead.Since folks find this a bit less idiomatic, and because
coreis - well - known as Rust'scorelibrary, I've merged allcoresubmodules into their parents, i.e.:rs_matter::coreis now simplyrs_matter(and thus no longerrs_matter::core::Matterbut justrs_matter::Matterrs_matter::transport::core->rs_matter::transportrs_matter::sc::core->rs_matter::scrs_matter::im::core->rs_matter::imrs_matter::dm::core->rs_matter::dminteraction_model->im,data_model->dm,secure_channel->scThis is probably the one change where I have second thoughts.
While Interaction Model and Secure Channel are commonly abbreviated in the Matter spec as
IMandSCrespectively - and - some constants in our codebase still carry aSC*/IM*prefix, these module names are a tad too short.On the other hand,
interaction_modelanddata_modelandsecure_channelare way too long, and they tend to be imported a lot. So I'm not 100% sure we should go with the abbreviated names, but I don't have better ideas for just slightly longer but more descriptive names either.If you have suggestions, I'm all ears!
All cluster-meta-data and handler implementations moved to
rs_matter::dm::clustersPreviously, some of the system clusters were in something called
rs_matter::dm::sdm(sdm?). Others were isrs_matter::dm:system_model. A few were directly inrs_matter::dm. I think it is just simpler to move all of these intors_matter::dm::clusters, and anyway 95% of these are "system" clusters necessary for the operation ofrs_matteranyway. The two exceptions are:unit_testing- we might move it totests/in future, but until we implement [TESTS] End-to-end tests #187 I would refrain from thaton_off- which is used in the examples onlySimilarly, I've renamed
rs_matter::dm::device_typestors_matter::dm::devicesandrs_matter::dm::root_endpointtors_matter::dm::endpointsto align withrs_matter::dm::clusters.Misc
A non-exhaustive list of smaller changes:
rs_matter::codecmoved tors_matter::utils::codecas it is independent of the rest of thers_mattercodebase. The rule forutilsis: everything which is not Matter-specific lives inutils. As in ourpin-initre-exports, ourpin-init-aware containers, etc. etc.codeccontains a single module for doing base38 serde hence it should live inutils;sc::commonandsc::status_reportmoved to justsc. So we finally have a symmetry between i.e.im::OpCodeandsc::OpCode;im::messagesandim::core::ibmoved toimdirectly. Just likeim::core. Ifimgets larger in size, we can always split bits and pieces back to separate, private sub-modules which are still re-exported withpub use private_submodule::*;in theimnamespace, for end-user simplicity, just like we do forrs_matter::tlvand forrs_matter::dm::types(see bullet point below for the latter);dm::objectsrenamed todm::typesand made private, and then re-exported in thedmnamespace directly, as theobjectsthing is too big to just be lumped as one big chunk of code indm). I don't thinkrs_matter::dm::objects::Handlerorrs_matter::dm::objects::Clusterorrs_matter::dm::objects::Attributewere meaningful. What is an "object" in the first place? (And yes, guilty as charged, as I think I introduced the "objects" module and sub-modules long ago). Also, these things are such a basic notion of the Data Model, that they deserve to live - from the perspective of the user - in the root of the Data Model anyway, i.e. directly underrs_matter::dm;rs_matter::crypto::crypto_mbedtls,rs_matter::crypto::crypto_rustcrypto(gotta love the 3-legged tautology!) and the rest of these are now (a) shortened to justrs_matter::crypto::mbedtls,rs_matter::crypto::rustcryptoand (b) are now private modules, where exactly one of these (depending on the activated crypto-provider) is re-exported as justrs_matter::crypto;rs_matter::scwhich has its own little crypto-extension story,rs_matter::sc::crypto_mbedtls,rs_matter::sc::crypto_rustcryptoand so on were re-named asrs_matter::sc::crypto::mbedtls,rs_matter::sc::crypto::rustcryptoand so on and also re-exported as justrs_matter::sc::cryptoI'm sure I'm missing a few smaller changes, but hopefully the above covers the major stuff.