Skip to content

refactor: Extract supported_operations module from c8y_api#3277

Merged
Bravo555 merged 2 commits intothin-edge:mainfrom
Bravo555:refactor/2-operations-loading
Dec 3, 2024
Merged

refactor: Extract supported_operations module from c8y_api#3277
Bravo555 merged 2 commits intothin-edge:mainfrom
Bravo555:refactor/2-operations-loading

Conversation

@Bravo555
Copy link
Copy Markdown
Member

@Bravo555 Bravo555 commented Dec 2, 2024 •
edited
Loading

Proposed changes

c8y_api::smartrest::operations was moved to c8y_mapper_ext::supported_operations because:

  • it deals with loading operation files from the local filesystem, not c8y_api concern
  • it was inside smartrest module, while not having much to do with smartrest (ie. it only generated "114 Set supported operations" message)

Additionally the supported_operations module was divided into 2 parts:

  • supported_operations: loading, updating, managing the supported operation set, matching on this set, generating messages to set, etc. basically managing the collection and interacting with CumulocityConverter
  • new supported_operations::operation: contains the functions and types related to reading and parsing single operation files from the filesystem and field substitution

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@Bravo555 Bravo555 temporarily deployed to Test Pull Request December 2, 2024 12:18 — with GitHub Actions Inactive
@Bravo555 Bravo555 changed the title Extract supported_operations module from c8y_api refactor: Extract supported_operations module from c8y_api Dec 2, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.18987% with 62 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...y_mapper_ext/src/supported_operations/operation.rs 90.96% 21 Missing and 11 partials ⚠️
...ons/c8y_mapper_ext/src/supported_operations/mod.rs 89.20% 18 Missing and 12 partials ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 2, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
529 0 2 529 100 1h32m35.977756999s

@didier-wenzek didier-wenzek added the refactoring Developer value label Dec 2, 2024
didier-wenzek
didier-wenzek reviewed Dec 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this refactoring, except that I would have preferred a git mv crates/core/c8y_api/src/smartrest/operations.rs ... rather than a delete and two new files.

@Bravo555
Copy link
Copy Markdown
Member Author

Bravo555 commented Dec 2, 2024

I'm okay with this refactoring, except that I would have preferred a git mv crates/core/c8y_api/src/smartrest/operations.rs ... rather than a delete and two new files.

It was necessary to make some changes in supported_operations.rs in the first commit because of changes in the imports section (e.g. c8y_mapper_ext doesn't depend on mqtt_channel, but on tedge_mqtt_ext, etc.)

Despite that, when the first commit is viewed separately, it shows as a rename, so old and new is side by side and you can compare that way.

But I can remove the 2nd commit and make it into a separate PR if you'd like.

@didier-wenzek
Copy link
Copy Markdown
Contributor

didier-wenzek commented Dec 2, 2024

But I can remove the 2nd commit and make it into a separate PR if you'd like.

No, that's okay. The move in the first commit is what I was looking for.

didier-wenzek
didier-wenzek approved these changes Dec 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@Bravo555 Bravo555 added this pull request to the merge queue Dec 2, 2024
@Bravo555 Bravo555 removed this pull request from the merge queue due to a manual request Dec 2, 2024
albinsuresh
albinsuresh reviewed Dec 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except for the odd module placement.

crates/extensions/c8y_mapper_ext/src/supported_operations.rs
@@ -0,0 +1,501 @@
//! [Supported Operations API]
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just find the placement of this module a bit weird, as it is placed directly at the root of the crate whereas operations.rs is placed under supported_operations. Would it make sense to move this into a mod.rs under supported_operations?

Copy link
Copy Markdown
Member Author

@Bravo555 Bravo555 Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style of module placement was supported for some time, and I think it's better to use than the older style for 2 reasons:

  • if you split a submodule off a bigger module, you don't have to move the other file and rename it to mod.rs, which makes the diffs a bit smaller (bigger pro IMO)
  • the file name is still a module name, whereas with mod.rs for the module name you have to look at the name of the parent directory, but as the editor tab contains only a file name, you can end up with many mod.rs files, so it's a bit harder to keep track of (actually in vscode when two files have the same name then it will display a parent in the tab for context)

But as we're already using the older style (we have 38 mod.rs files) I will use this style for consistency.

@Bravo555 Bravo555 force-pushed the refactor/2-operations-loading branch from 75b9694 to cec76d0 Compare December 3, 2024 10:40
@Bravo555 Bravo555 temporarily deployed to Test Pull Request December 3, 2024 10:40 — with GitHub Actions Inactive
@Bravo555
Move supported operations module from c8y_api to c8y_mapper_ext
839aad4 
`c8y_api::smartrest::operations` was moved to
`c8y_mapper_ext::supported_operations` because:

- it deals with loading operation files from the local filesystem, not
  c8y_api concern
- it was inside `smartrest` module, while not having much to do with
  smartrest (ie. it only generated "114 Set supported operations"
  message)

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555
Extract operation submodule from supported_operations module
71a68b1 
The `supported_operations` module is divided into 2 parts:

- loading, updating, managing the supported operation set, matching on
  this set, etc. basically managing the collection
- new `supported_operations::operation` contains the functions and types
  related to reading and parsing single operation files from the
  filesystem

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
@Bravo555 Bravo555 force-pushed the refactor/2-operations-loading branch from cec76d0 to 71a68b1 Compare December 3, 2024 11:55
@Bravo555 Bravo555 temporarily deployed to Test Pull Request December 3, 2024 11:55 — with GitHub Actions Inactive
rina23q
rina23q approved these changes Dec 3, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the improvement. I knew that core/c8y_api/src/smartrest had many operation related stuff but unrelated to SmartREST, however, I couldn't change while implementing a feature. It was frustrating. Happy to see that it's now refactored.

@Bravo555 Bravo555 added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024
@Bravo555 Bravo555 added this pull request to the merge queue Dec 3, 2024
Merged via the queue into thin-edge:main with commit 98b0f1d Dec 3, 2024
@Bravo555 Bravo555 deleted the refactor/2-operations-loading branch December 3, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albinsuresh albinsuresh albinsuresh left review comments

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@rina23q rina23q rina23q approved these changes

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

Assignees

No one assigned

Labels

refactoring Developer value

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Bravo555 @didier-wenzek @albinsuresh @rina23q