Skip to content

feat: canonical command names #497

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.

Sign up for GitHub

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

Jump to bottom
Merged
AlexD10S merged 9 commits into from
Apr 3, 2025
Merged

feat: canonical command names #497

AlexD10S merged 9 commits into from
Apr 3, 2025

Conversation

@Daanvdplas
Copy link
Collaborator

@Daanvdplas Daanvdplas commented Mar 28, 2025 •
edited
Loading

Refactors the implementations for collecting events and data from executing commands.

Essentially we convert the command before it is executed in the primary event. After execution the cli returns additional data. When an error occurs the error is added as data.

@Daanvdplas Daanvdplas force-pushed the daan/feat-telemetry_canonical_command_names branch from 9f98288 to 6c538cd Compare March 28, 2025 15:37
@Daanvdplas Daanvdplas requested a review from peterwht March 31, 2025 16:17
peterwht
peterwht requested changes Apr 1, 2025
View reviewed changes
Copy link
Contributor

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

This is looking good!

One thing I noticed outside the changes in this PR (but are relevant), is that there are several commands that have the Null variant. This would probably be a good PR to fix those. These include commands such as call and clean.

It would also be great to show what the telemetry output is in Umami as well so you can see what's expected. Especially with a more complex command with a few arguments / features, etc.

@Daanvdplas Daanvdplas marked this pull request as ready for review April 1, 2025 19:13
peterwht
peterwht reviewed Apr 1, 2025
View reviewed changes
Copy link
Contributor

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

awesome PR! Great refactor.

I just left two small comments, and then after your answer I am ready to approve.

@Daanvdplas Daanvdplas requested a review from AlexD10S April 2, 2025 07:36
AlexD10S
AlexD10S reviewed Apr 2, 2025
View reviewed changes
Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Great! Thanks for tackling this.
Just need to fix a unit test and a clippy warning, and we’re good to go.

Also tagging @chungquantin — if this gets merged before #496, we have to be sure it follows the same format in Try Runtime.

@Daanvdplas
Copy link
Collaborator Author

Daanvdplas commented Apr 2, 2025

Also tagging @chungquantin — if this gets merged before #496, we have to be sure it follows the same format in Try Runtime.

I suggest I handle that after that is merged

@codecov
Copy link

codecov bot commented Apr 2, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.30921% with 65 lines in your changes missing coverage. Please review.

Project coverage is 78.88%. Comparing base (0de418b) to head (c0149d8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/pop-cli/src/commands/mod.rs 83.00% 26 Missing ⚠️
crates/pop-cli/src/commands/install/mod.rs 0.00% 8 Missing ⚠️
crates/pop-cli/src/main.rs 96.00% 8 Missing ⚠️
crates/pop-cli/src/commands/test/mod.rs 80.00% 2 Missing and 4 partials ⚠️
crates/pop-cli/src/commands/bench/mod.rs 63.63% 4 Missing ⚠️
crates/pop-cli/src/commands/build/mod.rs 86.20% 4 Missing ⚠️
crates/pop-cli/src/commands/up/mod.rs 78.94% 1 Missing and 3 partials ⚠️
crates/pop-cli/src/commands/build/spec.rs 0.00% 2 Missing ⚠️
crates/pop-cli/src/commands/new/contract.rs 66.66% 1 Missing ⚠️
crates/pop-cli/src/commands/test/contract.rs 66.66% 1 Missing ⚠️
... and 1 more 
@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
+ Coverage   78.36%   78.88%   +0.51%     
==========================================
  Files          95       98       +3     
  Lines       22339    22809     +470     
  Branches    22339    22809     +470     
==========================================
+ Hits        17507    17993     +486     
+ Misses       2680     2663      -17     
- Partials     2152     2153       +1
Files with missing lines Coverage Δ
crates/pop-cli/src/commands/call/contract.rs 80.96% <100.00%> (+0.35%) ⬆️
crates/pop-cli/src/commands/call/mod.rs 100.00% <100.00%> (ø)
crates/pop-cli/src/commands/clean.rs 83.51% <100.00%> (+0.18%) ⬆️
crates/pop-cli/src/commands/new/mod.rs 100.00% <100.00%> (ø)
crates/pop-cli/src/commands/new/pallet.rs 16.40% <ø> (ø)
crates/pop-cli/src/commands/new/parachain.rs 46.17% <ø> (ø)
crates/pop-cli/src/commands/up/contract.rs 40.95% <100.00%> (ø)
crates/pop-cli/src/commands/up/network.rs 6.81% <ø> (ø)
crates/pop-cli/src/common/mod.rs 100.00% <100.00%> (ø)
crates/pop-cli/src/commands/new/contract.rs 48.05% <66.66%> (ø)
... and 10 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

AlexD10S
AlexD10S approved these changes Apr 2, 2025
View reviewed changes
Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the quick fixes

@AlexD10S AlexD10S mentioned this pull request Apr 2, 2025
Merged
3 tasks
peterwht
peterwht approved these changes Apr 2, 2025
View reviewed changes
Copy link
Contributor

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

LGTM!

@Daanvdplas Daanvdplas force-pushed the daan/feat-telemetry_canonical_command_names branch from ceb95e9 to 28dcc4f Compare April 3, 2025 08:55
@Daanvdplas Daanvdplas requested a review from AlexD10S April 3, 2025 08:56
AlexD10S
AlexD10S approved these changes Apr 3, 2025
View reviewed changes
Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexD10S AlexD10S merged commit 72c1f30 into main Apr 3, 2025
20 checks passed
@AlexD10S AlexD10S deleted the daan/feat-telemetry_canonical_command_names branch April 3, 2025 10:05
chungquantin pushed a commit that referenced this pull request Apr 7, 2025
@Daanvdplas @chungquantin
feat: canonical command names (#497)
9b9a4ca 
* feat: canonical command names

* refactor: use types in stead of strings

* refactor: telemetry

* fixes

* review

* fix: telemetry test

* fix: test, clippy

* refactor: try runtime

* fmt
This was referenced May 1, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexD10S AlexD10S AlexD10S approved these changes

@peterwht peterwht peterwht approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Daanvdplas @AlexD10S @peterwht