Skip to content

Rework Generics of Transformer interface #447

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
merged 2 commits into from
Oct 21, 2022

Conversation

NarekDW
Copy link
Contributor

@NarekDW NarekDW commented Oct 18, 2022

@snuyanzin could you review generics in Transformer interface, please?

I've found a number of issues here:

  1. On the code below, Schema<?, ? extends OUT> means that OUT transform(IN input) function from Field interface could generate the value only of some specific type. For example for CsvTransformer class, OUT is defined as CharSequence, so it means that we can generate CSV only from CharSequence, we couldn't generate CSV (fields) from other objects (integer, double, boolean e.g.).
    Take a look on the test case testCsvWithDifferentObjects and testCsvWithDifferentObjectsFunction.
public interface Transformer<IN extends AbstractProvider<?>, OUT> {
    OUT apply(Object input, Schema<?, ? extends OUT> schema);
...
  1. Here, IN generic type should not extends AbstractProvider, because in fact that type corresponds to the
    Schema<IN, ...> interface then it is passed to the function OUT transform(IN input) from Field interface, that function takes an argument of IN type and feeds it to the functional interface, which generates the data based on the argument, therefore, the argument could be an object of any type.
    Take a look on the test case testCsvWithDifferentObjects and testCsvWithDifferentObjectsFunction.
public interface Transformer<IN extends AbstractProvider<?>, OUT> {
    ...
    String generate(List<IN> input, final Schema<IN, ? extends OUT> schema);
    ...
}
  1. I think all the functions from interface Transformer should return a data of type OUT

Otherwise, if we don't want to change the types definition, we have to try to adjust the code to that types.

@codecov-commenter
Copy link

Codecov Report

Merging #447 (fa62365) into main (35651df) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #447      +/-   ##
============================================
+ Coverage     93.00%   93.03%   +0.02%     
- Complexity     2094     2098       +4     
============================================
  Files           233      233              
  Lines          4373     4377       +4     
  Branches        449      449              
============================================
+ Hits           4067     4072       +5     
+ Misses          190      186       -4     
- Partials        116      119       +3     
Impacted Files Coverage Δ
.../net/datafaker/transformations/CsvTransformer.java 93.10% <100.00%> (+0.51%) ⬆️
...net/datafaker/transformations/JsonTransformer.java 88.04% <100.00%> (ø)
.../net/datafaker/transformations/SqlTransformer.java 66.66% <100.00%> (ø)
.../main/java/net/datafaker/service/FakerContext.java 87.17% <0.00%> (+2.56%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@snuyanzin
Copy link
Collaborator

awesome, thanks

@snuyanzin snuyanzin merged commit 2085cdf into datafaker-net:main Oct 21, 2022
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