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(generics): support generic response types #332

Merged
merged 9 commits into from
Mar 18, 2022
Merged

fix(generics): support generic response types #332

merged 9 commits into from
Mar 18, 2022

Conversation

mharkins-cosm
Copy link
Contributor

This is an attempt to address issue #204. Because the nested generic objects must be checked dynamically for names, I had to get rid of the NAME constant and replace it with a name() method. Then it's just including named type parameter names in the parent name surrounded by angle brackets (<>).

@tiagolobocastro
Copy link
Collaborator

@mharkins-cosm could you please rebase so we can run the tests with gha?
I think this would be a breaking change to the API, so we'd have to bump up the version and add this to the changelog.

@mharkins-cosm
Copy link
Contributor Author

@mharkins-cosm could you please rebase so we can run the tests with gha? I think this would be a breaking change to the API, so we'd have to bump up the version and add this to the changelog.

Done

@tiagolobocastro
Copy link
Collaborator

@mharkins-cosm could you please rebase so we can run the tests with gha? I think this would be a breaking change to the API, so we'd have to bump up the version and add this to the changelog.

Done

Cool, it's running now. Could you please also add tests for generic response types and also update the changelog? Thanks

@mharkins-cosm
Copy link
Contributor Author

@mharkins-cosm could you please rebase so we can run the tests with gha? I think this would be a breaking change to the API, so we'd have to bump up the version and add this to the changelog.

Done

Cool, it's running now. Could you please also add tests for generic response types and also update the changelog? Thanks

Yes, I will. I'm sorry I haven't had time to get to it yet. It's on my todo list!

@mharkins-cosm
Copy link
Contributor Author

@tiagolobocastro Finally got to adding the requested test and changelog. Would you mind having a look and kicking of the checks if it looks good to you?

@mharkins-cosm
Copy link
Contributor Author

saw the lint check failed. i think i should have that fixed now.

vec![#(#type_params::name()),*]
.iter()
.filter(|n| n.is_some())
.map(|n| n.as_ref().unwrap().clone()).collect::<Vec<String>>()
Copy link
Collaborator

Choose a reason for hiding this comment

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

could use filter_map here? also maybe into_iter instead of iter avoiding having to write the clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm... i get a warning for into_iter, but can use filter_map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tiagolobocastro
Copy link
Collaborator

saw the lint check failed. i think i should have that fixed now.

cool I kicked the GHA, will have a another look tomorrow

@mharkins-cosm
Copy link
Contributor Author

Funny the clippy warning is not part of this PR, nor does it appear to be accurate. Do you want me to add an allow annotation anyway, to allow the check to pass?

@tiagolobocastro
Copy link
Collaborator

Funny the clippy warning is not part of this PR, nor does it appear to be accurate. Do you want me to add an allow annotation anyway, to allow the check to pass?

That'd be great if you don't mind 👍

@tiagolobocastro tiagolobocastro merged commit a0e9d32 into paperclip-rs:master Mar 18, 2022
@mharkins-cosm
Copy link
Contributor Author

Thanks very much @tiagolobocastro ! Any idea when it'll be released, along with the actix 4 support?

@tiagolobocastro
Copy link
Collaborator

No, thank you! I'll probably try to publish it this weekend.

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