Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 24, 2022

Description

This PR introduces a feature that allows us to wrap REST request parameters under any arbitrary model and add them to a RestRequest instance via an extension method.

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@alexeyzimarev
Copy link
Member

Doesn't it overlap with AddObject?

@alexeyzimarev
Copy link
Member

I like this implementation better, and I think it could be another version of AddObject, which does the same by adding properties as GetOrPost parameters, but doesn't allow to specify the type.

@ghost
Copy link
Author

ghost commented Sep 27, 2022

I like this implementation better, and I think it could be another version of AddObject, which does the same by adding properties as GetOrPost parameters, but doesn't allow to specify the type.

Sounds good to me. Sorry for the delayed response, my schedule has been rather tight lately. I'll be working on it this weekend.

Also worth asking: What do you think of replacing the current implementation with this one as well? We could benefit from caching the instance properties for a given type and filtering them accordingly later on. I'll make sure to provide the benchmarks if that sounds good to you. This won't break the existing API, as the method signature will be left unchanged.

@stale
Copy link

stale bot commented Oct 29, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 29, 2022
@repo-ranger
Copy link

repo-ranger bot commented Oct 29, 2022

⚠️ This issue has been marked wontfix and will be closed in 3 days

@repo-ranger repo-ranger bot closed this Nov 1, 2022
@alexeyzimarev alexeyzimarev reopened this Nov 7, 2022
@dnfadmin
Copy link

dnfadmin commented Nov 7, 2022

CLA assistant check
All CLA requirements met.

@alexeyzimarev
Copy link
Member

This time the delay is on my side, apologies.

The problem with replacing AddObject with a generic method is that you aren't always able to know the object type. I am sure that your implementation is much faster, but I don't think the existing method should be (or can be) replaced with this one without consequences. So, I'd rather have both of them.

@ghost
Copy link
Author

ghost commented Nov 18, 2022

This time the delay is on my side, apologies.

The problem with replacing AddObject with a generic method is that you aren't always able to know the object type. I am sure that your implementation is much faster, but I don't think the existing method should be (or can be) replaced with this one without consequences. So, I'd rather have both of them.

I see, that makes sense. Created a new PR with some changes based on this conversation. :)
#1974

@ghost ghost closed this Nov 18, 2022
This pull request was closed.
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.

3 participants