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

aya-log: Unify IP format hints into one, repsesent it by :i token #599

Merged
merged 1 commit into from
May 27, 2023

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented May 2, 2023

Having separate format hints and tokens per IP address family is unnecessary, since they are represented by different types and we handle format hints for each type separately. So we can just have one format hint.

Also, we should be consistent with the format strings grammar in Rust[0]. The type token, which is mapped to formatting traits, usually consists of one letter[1] (and optional ? for Debug trait, but that doesn't matter for us). It shouldn't consist of multiple letters. Our :ipv4 and :ipv6 tokens were clearly breaking that convention, so we should rather switch to something with one letter - hence :i.

[0] https://doc.rust-lang.org/std/fmt/#syntax
[1] https://doc.rust-lang.org/std/fmt/#formatting-traits

@netlify
Copy link

netlify bot commented May 2, 2023

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9e0a3aa
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64510d35eb18390008529bce
😎 Deploy Preview https://deploy-preview-599--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@vadorovsky
Copy link
Member Author

I'm going to change other incorrect tokens (e.g. :mac) in separate PRs.

Having separate format hints and tokens per IP address family is
unnecessary, since they are represented by different types and we handle
format hints for each type separately. So we can just have one format
hint.

Also, we should be consistent with the format strings grammar in
Rust[0]. The `type` token, which is mapped to formatting traits, usually
consists of one letter[1] (and optional `?` for `Debug` trait, but that
doesn't matter for us). It shouldn't consist of multiple letters. Our
`:ipv4` and `:ipv6` tokens were clearly breaking that convention, so we
should rather switch to something with one letter - hence `:i`.

[0] https://doc.rust-lang.org/std/fmt/#syntax
[1] https://doc.rust-lang.org/std/fmt/#formatting-traits
Copy link

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I additionally tested this out locally on my project, and the results look good 👍

@alessandrod alessandrod merged commit 84e5e28 into aya-rs:main May 27, 2023
13 checks passed
rbartlensky added a commit to rbartlensky/aya that referenced this pull request May 27, 2023
aya-log-ebpf-macros was failing to compile because it was referencing
a couple of `DisplayHint` variants that no longer exist. These were
removed in aya-rs#599.

```
    Compiling aya-log-ebpf-macros v0.1.0 (/home/robert/aya/aya-log-ebpf-macros)
error[E0599]: no variant or associated item named `Ipv4` found for enum `DisplayHint` in the current scope
  --> aya-log-ebpf-macros/src/expand.rs:93:22
   |
93 |         DisplayHint::Ipv4 => parse_str("::aya_log_ebpf::macro_support::check_impl_ipv4"),
   |                      ^^^^ variant or associated item not found in `DisplayHint`

error[E0599]: no variant or associated item named `Ipv6` found for enum `DisplayHint` in the current scope
  --> aya-log-ebpf-macros/src/expand.rs:94:22
   |
94 |         DisplayHint::Ipv6 => parse_str("::aya_log_ebpf::macro_support::check_impl_ipv6"),
   |                      ^^^^ variant or associated item not found in `DisplayHint`

For more information about this error, try `rustc --explain E0599`.
```
rbartlensky added a commit to rbartlensky/aya that referenced this pull request May 28, 2023
aya-log-ebpf-macros was failing to compile because it was referencing
a couple of `DisplayHint` variants that no longer exist. These were
removed in aya-rs#599.

```
    Compiling aya-log-ebpf-macros v0.1.0 (/home/robert/aya/aya-log-ebpf-macros)
error[E0599]: no variant or associated item named `Ipv4` found for enum `DisplayHint` in the current scope
  --> aya-log-ebpf-macros/src/expand.rs:93:22
   |
93 |         DisplayHint::Ipv4 => parse_str("::aya_log_ebpf::macro_support::check_impl_ipv4"),
   |                      ^^^^ variant or associated item not found in `DisplayHint`

error[E0599]: no variant or associated item named `Ipv6` found for enum `DisplayHint` in the current scope
  --> aya-log-ebpf-macros/src/expand.rs:94:22
   |
94 |         DisplayHint::Ipv6 => parse_str("::aya_log_ebpf::macro_support::check_impl_ipv6"),
   |                      ^^^^ variant or associated item not found in `DisplayHint`

For more information about this error, try `rustc --explain E0599`.
```
alessandrod pushed a commit that referenced this pull request May 28, 2023
aya-log-ebpf-macros was failing to compile because it was referencing
a couple of `DisplayHint` variants that no longer exist. These were
removed in #599.

```
    Compiling aya-log-ebpf-macros v0.1.0 (/home/robert/aya/aya-log-ebpf-macros)
error[E0599]: no variant or associated item named `Ipv4` found for enum `DisplayHint` in the current scope
  --> aya-log-ebpf-macros/src/expand.rs:93:22
   |
93 |         DisplayHint::Ipv4 => parse_str("::aya_log_ebpf::macro_support::check_impl_ipv4"),
   |                      ^^^^ variant or associated item not found in `DisplayHint`

error[E0599]: no variant or associated item named `Ipv6` found for enum `DisplayHint` in the current scope
  --> aya-log-ebpf-macros/src/expand.rs:94:22
   |
94 |         DisplayHint::Ipv6 => parse_str("::aya_log_ebpf::macro_support::check_impl_ipv6"),
   |                      ^^^^ variant or associated item not found in `DisplayHint`

For more information about this error, try `rustc --explain E0599`.
```
gingk1212 added a commit to gingk1212/book that referenced this pull request May 29, 2023
`{:ipv4}` has been replaced with `{:i}` in aya-rs/aya#599.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants