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 AsRef and AsMut derives #286

Merged
merged 33 commits into from
Aug 18, 2023

Conversation

MegaBluejay
Copy link
Contributor

@MegaBluejay MegaBluejay commented Aug 9, 2023

Synopsis

This PR is a part of replacing all attributes having syn::Meta syntax to custom parsing, similarly to #241, #248

Paves the way for #285, and possibly other enhancements such as an opt-in #123

Solution

Implement custom attribute parsing without State

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

@MegaBluejay MegaBluejay marked this pull request as ready for review August 9, 2023 12:57
@MegaBluejay MegaBluejay marked this pull request as draft August 9, 2023 12:57
Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Just to be clear, this isn't adding any new features yet right? Only doing refactoring to allow future features. (I like that approach, just making sure I understand correctly)

impl/src/as_ref.rs Outdated Show resolved Hide resolved
impl/src/as_ref.rs Outdated Show resolved Hide resolved
impl/src/as_ref.rs Outdated Show resolved Hide resolved
@MegaBluejay
Copy link
Contributor Author

Only doing refactoring to allow future features.

Yes, that's correct, this PR is refactoring only

@MegaBluejay MegaBluejay changed the title Rework AsRef macro input parser Rework AsRef and AsMut macro input parser Aug 10, 2023
@MegaBluejay
Copy link
Contributor Author

Since they're essentially identical, I think it makes sense to use the same deriving function for AsRef and AsMut.

I can't think of any hypothetical features that should apply to only one of them, and this would facilitate keeping them in sync.

@MegaBluejay MegaBluejay marked this pull request as ready for review August 10, 2023 16:32
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@MegaBluejay not bad, but needs polishing.

I've restructured modules in the way we have for fmt macros.

Also, please look at tests/as_ref.rs and tests/as_mut.rs, whether it makes sense to extend their test cases to cover more edge cases.

impl/src/ref_conv.rs Outdated Show resolved Hide resolved
impl/src/ref_conv.rs Outdated Show resolved Hide resolved
impl/src/ref_conv.rs Outdated Show resolved Hide resolved
impl/src/ref_conv.rs Outdated Show resolved Hide resolved
@tyranron
Copy link
Collaborator

@JelteF the interesting question:

Since AsRef and AsMut are effectively the same regarding implementation, should we unite them under a single as_ref Cargo feature as we do for Display-like derives?

I think it's quite reasonable. May be done as a separate PR, though, after merging this one.

@JelteF
Copy link
Owner

JelteF commented Aug 17, 2023

I think it's quite reasonable. May be done as a separate PR, though, after merging this one.

Yes sounds good to me. But indeed let's do that in a follow-up PR.

impl/src/as/mod.rs Outdated Show resolved Hide resolved
@JelteF
Copy link
Owner

JelteF commented Aug 17, 2023

@tyranron feel free to sign off on after you are happy. I took a quick look and it looks reasonable to me.

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@MegaBluejay I've heavily refactored your implementation. Consider the changes.

Regarding the implementation itself, it's good enough for merging already. However, the tests are still lacking features coverage very much, which I requested in previous review. No tests for #[as_ref] and #[as_ref(skip)] were added at all. I've restructured the existing tests in as_mut, so they have some logical sequence, but I think it's not good enough yet.

Let's break it by modules:

mod single_field {
    mod tuple {
          // Clean
          // Forward
          // FieldForward
          mod generic {
              // Clean
              // Forward
              // FieldForward
          }
    }
    mod named {
          // Clean
          // Forward
          // FieldForward
          mod generic {
              // Clean
              // Forward
              // FieldForward
          }
    }
}
mod multi_field {
    mod tuple {
          // Clean
          // Forward
          // Field (multiple impls)
          // Ignored (multiple impls)
          mod generic {
              // Clean
              // Forward
              // Field (multiple impls)
              // Ignored (multiple impls)
          }
    }
    mod named {
          // Clean
          // Forward
          // Field (multiple impls)
          // Ignored (multiple impls)
          mod generic {
              // Clean
              // Forward
              // Field (multiple impls)
              // Ignored (multiple impls)
          }
    }
}
```

impl/src/as/mod.rs Outdated Show resolved Hide resolved
return Err(syn::Error::new(
attr.attr.span(),
format!(
"`#[{0}(ignore)]` cannot be used in the same struct as other `#[{0}(...)]` \
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay in other derive macros (see compile_fail for them) we always talk about skip attribute and keep ignore as an alias. Why do the opposite here?

I've created utils::skip::Attribute, which can be reused, so we don't repeat the same logic everywhere.

After merging this PR, please open a new one which refactors From/Into/Debug/Display to reuse utils::skip and utils::forward where applicable.

impl/src/as/mod.rs Outdated Show resolved Hide resolved
@MegaBluejay MegaBluejay changed the title Rework AsRef and AsMut macro input parser Refactor AsRef and AsMut derives Aug 18, 2023
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@MegaBluejay good! 👍

After merging this PR, please do as subsequent ones:

  1. Since AsRef and AsMut are effectively the same regarding implementation, should we unite them under a single as_ref Cargo feature as we do for Display-like derives?

    I think it's quite reasonable. May be done as a separate PR, though, after merging this one.

  2. After merging this PR, please open a new one which refactors From/Into/Debug/Display to reuse utils::skip and utils::forward where applicable.

tests/as_mut.rs Show resolved Hide resolved
@tyranron tyranron added this to the 1.0.0 milestone Aug 18, 2023
@tyranron tyranron merged commit 6a444a3 into JelteF:master Aug 18, 2023
16 checks passed
tyranron pushed a commit that referenced this pull request Aug 21, 2023
First suggested
[here](#286 (comment)).


## Synopsis

`AsRef` and `AsMut` derives are effectively the same regarding
implementation, so using separate features for them doesn't make sense.


## Solution

Use the `as_ref` feature for both derives.
tyranron added a commit that referenced this pull request Aug 22, 2023
First suggested
[here](#286 (comment)).


## Synopsis

There's some repetition around parsing the same attributes in different
derives.


## Solution

Refactor `Into`, `From`, and `Debug` derives to use the `skip` and
`forward` attribute parsing from `utils` module.


## Additionally

Update documentation by mentioning an `ignore` alias for a`skip` attribute.


Co-authored-by: Kai Ren <tyranron@gmail.com>
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