Skip to content

Use params modifier for methods in RequestDescriptorBase #4579

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

Merged

Conversation

tkirill
Copy link
Contributor

@tkirill tkirill commented Apr 13, 2020

Hello!

This PR tries to fix #4471.

The only place I found to fix is RequestDescriptorBase.FilterPath so either this is a really low hanging fruit or I'm completely missing something. I searched for potential places to fix with naive search for (string[] and with looking at usings of TypeHighLevel in views.

I noticed that methods in Descriptors.*.cs already have params modifier and then found that it is implemented with .DescriptorArgumentType in Descriptor.cshtml. Looks like it is suitable for RequestDescriptorBase too.

I'm not sure if Descriptors.cs should be commited. The whole process of testing codegen is a little bit confusing and I didn't figured out the right way to run tests locally for my changes (and will be glad if someone teach me). Anyway it was a very interesting exercise to look through the code generation tool.

@elasticmachine

This comment has been minimized.

@Mpdreamz
Copy link
Member

Thanks @tkirill for taking on this challenge!

Also your input into the documentation of this process is invaluable we'll work on improving our Contributing.md 👍

This is not a breaking change as per: https://docs.microsoft.com/en-us/dotnet/core/compatibility/#code-changes so LGTM.

I do think wherever we accept params we should create a non allocating overload with IEnumerable too. Are you up for the challenge to update this PR? If not no sweat we'll amend afterwards.

@tkirill
Copy link
Contributor Author

tkirill commented Apr 16, 2020

@Mpdreamz Yes, sure I want, this is very interesting challenge. At least I'll try.

I guess I don't fully understand the scope of the task. This is how I see it:

  • For each API endpoint there are two parts: low-level (RequestParameters.*.cs, ElasticLowLevelClient.*.cs) in Elasticsearch.Net and high-level in Nest. This task is about high-level code.
  • High-level code can be auto-generated (Descriptors.*.cs, Requests.*.cs, ElasticClient.*.cs) and manual-writed. Manual-writed descriptors always have both IEnumerable and params overloads. Auto-generated descriptors have only params version and we want to add IEnumerable overload.
  • So the targets are Descriptor.cshtml and RequestDescriptorBase.cshtml

If this is far from being the whole story then I guess it will take unreasonably much of your time to explain me what to do. In this case I'll be happy to stop on current tiny contribution and just wait and look at full solution later. And if everything is actually fine then I can do it and it will be very interesting.

@Mpdreamz
Copy link
Member

Hey @tkirill your suggested scope is the whole nine yards :)

I was talking about simply extending this PR so that ``src/Nest/Descriptors.cs also includes the overload. Keeps the PR small and manageable 👍

@tkirill tkirill force-pushed the params-modifier-RequestDescriptorBase branch from 6d7429a to 6a3e05e Compare April 18, 2020 18:41
Effectively it works for FilterPath only.  Other methods doesn't have array parameters.

fix elastic#4471
@tkirill tkirill force-pushed the params-modifier-RequestDescriptorBase branch from 6a3e05e to 0440bf1 Compare April 18, 2020 19:00
@tkirill
Copy link
Contributor Author

tkirill commented Apr 18, 2020

Hey @Mpdreamz I added IEnumerable overload.

@codebrain
Copy link
Contributor

This is great @tkirill! thank you for your contribution.
I will get this merged in and ready for 7.7.0.

@codebrain codebrain merged commit c180a23 into elastic:master Apr 20, 2020
codebrain pushed a commit that referenced this pull request Apr 20, 2020
Effectively it works for FilterPath only.  Other methods doesn't have array parameters.

Fixes #4471

(cherry picked from commit c180a23)
codebrain pushed a commit that referenced this pull request Apr 20, 2020
Effectively it works for FilterPath only.  Other methods doesn't have array parameters.

Fixes #4471

(cherry picked from commit c180a23)
@codebrain
Copy link
Contributor

Backported to 7.x and 7.7

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.

Improve LLC descriptors generation
4 participants