Skip to content

Conversation

@stanlp1
Copy link
Contributor

@stanlp1 stanlp1 commented Sep 28, 2023

No description provided.

@stanlp1 stanlp1 requested a review from a team September 28, 2023 22:37
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

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

LGTM! filterBy is very complex so taking in a JSON string might be the way to go (current implementation). @jjl014 wdyt?

Probably not related to this PR but mvn wasn't working for me with the error Target option 7 is no longer supported. Use 8 or later.. I had to change source and target to 8.

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-compiler-plugin</artifactId>
  <version>3.0</version>
  <configuration>
    <source>8</source>
    <target>8</target>
  </configuration>
</plugin>

Copy link
Contributor

@jjl014 jjl014 left a comment

Choose a reason for hiding this comment

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

Nice, this looks great to me as well! 🚀

@esezen I was able to run the tests while still on version 7. I changed it to 8 as well and ran mvn install with no issues. We can update it to version 8 in a separate PR so it's consistent for everyone. 👍

@stanlp1 stanlp1 merged commit 4a1d8db into master Oct 5, 2023
@stanlp1 stanlp1 deleted the csl-2831-java-sdk-add-support-for-filter_by-in-variations-mapping branch October 5, 2023 21:02
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.

4 participants