Skip to content
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.

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

[refactor]: update iroha_telemetry_derive to use syn2 #4168

Merged
merged 4 commits into from
Jan 15, 2024
Merged

[refactor]: update iroha_telemetry_derive to use syn2 #4168

merged 4 commits into from
Jan 15, 2024

Conversation

VAmuzing
Copy link
Contributor

@VAmuzing VAmuzing commented Dec 25, 2023

Description

Update with refactoring to remove proc_macro_error in favour of our Emitter wrapper

Linked issue

Closes #3909

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Dec 25, 2023
@VAmuzing VAmuzing self-assigned this Dec 25, 2023
@VAmuzing
Copy link
Contributor Author

Need to update failure tests as well if we proceed with these errors

Copy link
Contributor

@DCNick3 DCNick3 left a comment

Choose a reason for hiding this comment

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

Code looks good (aside from minor nitpicks)!

Though would like to look at the changed error messages (if any)

telemetry/derive/src/lib.rs Outdated Show resolved Hide resolved
telemetry/derive/src/lib.rs Outdated Show resolved Hide resolved
@VAmuzing
Copy link
Contributor Author

Code looks good (aside from minor nitpicks)!

Though would like to look at the changed error messages (if any)

The errors contain additional lines as emitter does not abrupt macro compilation like abort! did.
This is the example:

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Function must have at least one argument of type WorldStateView.
--> tests/ui_fail/no_args.rs:4:1
|
4 | fn execute() -> Result<(), ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Function must have at least one argument of type WorldStateView.
--> tests/ui_fail/no_args.rs:4:1
|
4 | fn execute() -> Result<(), ()> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: At least one argument must be a WorldStateView.
--> tests/ui_fail/no_args.rs:3:1
|
3 | #[metrics(+"test_query", "another_test_query_without_timing")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this error originates in the attribute macro metrics (in Nightly builds, run with -Z macro-backtrace for more info)
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

What do you think about printing additional errors?

@DCNick3
Copy link
Contributor

DCNick3 commented Dec 26, 2023

This... doesn't seem right. Seems like the missing wsv argument is getting double-reported.

I guess you don't need this check, as it's also checked later: https://github.com/hyperledger/iroha/pull/4168/files#diff-6fe38d7a884371cd14b17c23142ac64383dee34a81b9f87a5b81e3eeebc25ff7R162-R168

@VAmuzing
Copy link
Contributor Author

This... doesn't seem right. Seems like the missing wsv argument is getting double-reported.

I guess you don't need this check, as it's also checked later: https://github.com/hyperledger/iroha/pull/4168/files#diff-6fe38d7a884371cd14b17c23142ac64383dee34a81b9f87a5b81e3eeebc25ff7R162-R168

Adjusted stderr and added early return as the next one refers to the macro signature.
This was the error about empty signature is more clear.

@VAmuzing VAmuzing mentioned this pull request Jan 3, 2024
12 tasks
@AlexStroke AlexStroke self-assigned this Jan 9, 2024
@AlexStroke AlexStroke closed this Jan 9, 2024
@AlexStroke AlexStroke reopened this Jan 9, 2024
@DCNick3 DCNick3 self-assigned this Jan 10, 2024
Signed-off-by: VAmuzing <amuzik95@gmail.com>
Signed-off-by: VAmuzing <amuzik95@gmail.com>
Signed-off-by: VAmuzing <amuzik95@gmail.com>
@Erigara Erigara self-assigned this Jan 15, 2024
@VAmuzing VAmuzing merged commit 5d4082f into hyperledger-iroha:iroha2-dev Jan 15, 2024
7 of 11 checks passed
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this pull request Jan 22, 2024
…oha#4168)

Signed-off-by: VAmuzing <amuzik95@gmail.com>
Signed-off-by: Asem-Abdelhady <asemshawkey@gmail.com>
Asem-Abdelhady pushed a commit to Asem-Abdelhady/iroha that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants