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

Fix openapi deserialize #70

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Conversation

r3bu1ld3r
Copy link
Contributor

  • Solves Fix OpenApi deserialize #69
  • Fixes for compiler and clippy warnings
  • Changed openapi field type from '&static str to Cow<'static, str> for the relaxing requirement for deserialized data (actual for the case when you trying to deserialize data which not previously statically included in your binary)

@Wicpar
Copy link
Collaborator

Wicpar commented Sep 16, 2023

Thank you for the fix.

The mut cannot be removed in all these functions. I don't know why there was a warning for you.

I'll do the merge once the transform mut changes are removed.

@r3bu1ld3r
Copy link
Contributor Author

r3bu1ld3r commented Sep 17, 2023

Thank you for the review @Wicpar !

I've reverted the commit with compiler warnings fixes, so you can merge the deserialize fix. Now, during compilation there are warnings

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:343:20
    |
343 |     pub fn summary(mut self, desc: &str) -> Self {
    |                    ----^^^^
    |                    |
    |                    help: remove this `mut`
    |
    = note: `#[warn(unused_mut)]` on by default

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:350:24
    |
350 |     pub fn description(mut self, desc: &str) -> Self {
    |                        ----^^^^
    |                        |
    |                        help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:504:20
    |
504 |     pub fn summary(mut self, desc: &str) -> Self {
    |                    ----^^^^
    |                    |
    |                    help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:511:24
    |
511 |     pub fn description(mut self, desc: &str) -> Self {
    |                        ----^^^^
    |                        |
    |                        help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:611:32
    |
611 |     pub fn default_response<R>(mut self) -> Self
    |                                ----^^^^
    |                                |
    |                                help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:642:40
    |
642 |     pub fn default_response_with<R, F>(mut self, transform: F) -> Self
    |                                        ----^^^^
    |                                        |
    |                                        help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:676:38
    |
676 |     pub fn response<const N: u16, R>(mut self) -> Self
    |                                      ----^^^^
    |                                      |
    |                                      help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:708:46
    |
708 |     pub fn response_with<const N: u16, R, F>(mut self, transform: F) -> Self
    |                                              ----^^^^
    |                                              |
    |                                              help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:744:44
    |
744 |     pub fn response_range<const N: u16, R>(mut self) -> Self
    |                                            ----^^^^
    |                                            |
    |                                            help: remove this `mut`

warning: variable does not need to be mutable
   --> crates/aide/src/transform.rs:778:52
    |
778 |     pub fn response_range_with<const N: u16, R, F>(mut self, transform: F) -> Self
    |                                                    ----^^^^
    |                                                    |
    |                                                    help: remove this `mut`

warning: variable does not need to be mutable
    --> crates/aide/src/transform.rs:1057:24
     |
1057 |     pub fn description(mut self, desc: &str) -> Self {
     |                        ----^^^^
     |                        |
     |                        help: remove this `mut`

warning: `aide` (lib) generated 11 warnings (run `cargo fix --lib -p aide` to apply 11 suggestions)
    Finished dev [unoptimized + debuginfo] target(s) in 9.22s

Does marking these functions at least with #[allow(unused_mut)] make sense for avoiding these warnings?

I've checked warnings for methods summary and description of TransformPathItem:

/// A transform helper that wraps [`TransformPathItem`].
#[must_use]
pub struct TransformPathItem<'t> {
    pub(crate) hidden: bool,
    pub(crate) path: &'t mut PathItem,
}

impl<'t> TransformPathItem<'t> {
    /// Create a new transform helper.
    pub fn new(path: &'t mut PathItem) -> Self {
        Self {
            hidden: false,
            path,
        }
    }

    /// Hide the path from the documentation.
    ///
    /// This is taken into account by generators provided
    /// by this library.
    ///
    /// Hiding an item causes it to be ignored
    /// completely, there is no way to restore or "unhide" it afterwards.
    #[tracing::instrument(skip_all)]
    pub fn hidden(mut self, hidden: bool) -> Self {
        self.hidden = hidden;
        self
    }

    /// Provide a summary for the path.
    #[tracing::instrument(skip_all)]
    pub fn summary(mut self, desc: &str) -> Self {
        self.path.summary = Some(desc.into());
        self
    }

    /// Provide a description for the path.
    #[tracing::instrument(skip_all)]
    pub fn description(mut self, desc: &str) -> Self {
        self.path.description = Some(desc.into());
        self
    }
    
//... skipped
}

It really doesn't need mut self here because the path field was already taken over mutable reference and can be changed even without mut self. I assume other warnings may have the same reason.

@Wicpar
Copy link
Collaborator

Wicpar commented Sep 17, 2023

It's really odd, i may be wrong but a non mut parameter should not be assignable, even inner members unless there is an unsafe cell. Maybe self is an exception.
Let me check and i'll merge it with the mut changes if it really is in the standard.

@Wicpar
Copy link
Collaborator

Wicpar commented Sep 18, 2023

Ah i see indeed it is a mut ref already, as you said. Somehow i didn't see that. I'll merge the mut changes too

@r3bu1ld3r
Copy link
Contributor Author

Hey @Wicpar I've returned the original commit for a cleaner commit history, so you can merge all changes.

@Wicpar Wicpar merged commit e3bbde1 into tamasfe:master Sep 18, 2023
2 checks passed
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.

2 participants