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

[Core] Support multiple doc strings types with same content type (#2343) #2421

Conversation

PostelnicuGeorge
Copy link
Contributor

@PostelnicuGeorge PostelnicuGeorge commented Nov 21, 2021

Is your pull request related to a problem? Please describe.
It's the implementation to: #2343

Describe the solution you have implemented
With this implementation is now possible to have DocStrings of default type ("") or a particular contentType like "application/json" to have multiple DocStringTypes (different return types). This gives leverage for users to take full advantage of Jackson ObjectMapper conversions.
Tests were implemented and updated to cover the new situations where it should be allowed to have multiple DocStringTypes per same contentType, this creates much more concise transformers but doesn't allow any more ambiguity around Object as a return type.

@mpkorstanje
Copy link
Contributor

But given that I saw that the java8 project is marked for deprecation I'm awaiting further details on how to deal with this one.

We're planning for deprecation but the project hasn't been deprecated yet - due to a lack of replacement. Until it is removed cucumber-java8 should remain functioning in the same way as cucumber-java.

@PostelnicuGeorge
Copy link
Contributor Author

But given that I saw that the java8 project is marked for deprecation I'm awaiting further details on how to deal with this one.

We're planning for deprecation but the project hasn't been deprecated yet - due to a lack of replacement. Until it is removed cucumber-java8 should remain functioning in the same way as cucumber-java.

Got it, will investigate that method and see if I manage to make it return the actual GenericReturnType of DocStringType lambdas.

@PostelnicuGeorge PostelnicuGeorge marked this pull request as draft November 24, 2021 10:21
@PostelnicuGeorge PostelnicuGeorge force-pushed the feature/PG-2343_support_different_doc_string_types branch from 9aa111a to 3fd8a35 Compare November 25, 2021 12:30
@PostelnicuGeorge
Copy link
Contributor Author

Fixed the "type erasure" issue, all test pass, I'm able to do a "clean install" at the project level.

@PostelnicuGeorge PostelnicuGeorge marked this pull request as ready for review November 25, 2021 12:31
- Replaces the use of `Objects.isNull` with regular null checks.
- Removes the use of `Optional` and entangled methods by making lookup return a list
@mpkorstanje mpkorstanje force-pushed the feature/PG-2343_support_different_doc_string_types branch 2 times, most recently from 02d2ffa to b27d69d Compare November 28, 2021 17:53
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Heya, looks really good!

I've pushed some changes to resolve some of nitpicks I had with the DocStringTypeRegistry and DocStringTypeRegistryDocStringConverter. By making docStringTypeRegistry.lookup return a list it is possible to handle all error cases at once.

I've got some questions about the other changes.

@mpkorstanje
Copy link
Contributor

Btw, don't worry about force pushing. The default is squash merge so we don't have to keep branches clean.

@codecov
Copy link

codecov bot commented Nov 28, 2021

Codecov Report

Merging #2421 (8f9f092) into main (5598e34) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 8f9f092 differs from pull request most recent head 1bcc7b9. Consider uploading reports for the commit 1bcc7b9 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2421      +/-   ##
============================================
+ Coverage     83.61%   83.66%   +0.05%     
- Complexity     2634     2646      +12     
============================================
  Files           317      317              
  Lines          9312     9331      +19     
  Branches        905      906       +1     
============================================
+ Hits           7786     7807      +21     
  Misses         1191     1191              
+ Partials        335      333       -2     
Impacted Files Coverage Δ
...o/cucumber/java8/Java8DocStringTypeDefinition.java 60.00% <ø> (ø)
...a8/src/main/java/io/cucumber/java8/LambdaGlue.java 82.63% <ø> (ø)
...a/io/cucumber/docstring/DocStringTypeRegistry.java 100.00% <100.00%> (ø)
...tring/DocStringTypeRegistryDocStringConverter.java 100.00% <100.00%> (ø)
.../io/cucumber/java/JavaDocStringTypeDefinition.java 100.00% <100.00%> (+9.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5598e34...1bcc7b9. Read the comment docs.

@PostelnicuGeorge
Copy link
Contributor Author

PostelnicuGeorge commented Nov 29, 2021

I have added a test to check that if we use the same parameter type like List of Grocery in a step definition, we're able to use interchangeably that a DocString or DataTable can be used that convert to that particular type. Is there a better place to put this kind of test?
io.cucumber.core.stepexpression.StepExpressionFactoryTest#docstring_and_datatable_match_same_step_definition

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Nov 30, 2021

I'd say that StepExpressionFactoryTest is the right place for that test. It's where both converters come together.

Now all that is left now is to add an example to the documentation to illustrate this usecase.

https://github.com/cucumber/cucumber-jvm/tree/main/java#docstring-type

I think one one example where two json doc strings are converted to different objects would be sufficient.

@PostelnicuGeorge
Copy link
Contributor Author

I have added the example, is there something else remaining? What's the process now going further?
Also is there any PR / Change Request that you see fit I can handle, I have some spare time at the end of this year.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Dec 2, 2021

I'll have to find the time to give it another review and then it will presumably be merged and released.

All the issues tagged with help wanted should be one you could pick up.

@mpkorstanje mpkorstanje force-pushed the feature/PG-2343_support_different_doc_string_types branch from 8f9f092 to 1bcc7b9 Compare December 11, 2021 13:15
@mpkorstanje mpkorstanje merged commit 8aa7859 into cucumber:main Dec 11, 2021
@mpkorstanje mpkorstanje changed the title Support multiple DocStringTypes with the same contentType (#2343) [Core] Support multiple doc strings types with same content type (#2343) Dec 11, 2021
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