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

monocle3, scala3 upgrade #336

Closed
wants to merge 4 commits into from
Closed

Conversation

inanme
Copy link

@inanme inanme commented Jul 25, 2023

I guess it's customary to create a PR for Scala3 & Monocle3 upgrades. This PR removes all deprecated Monocle API usages.

@@ -39,6 +39,6 @@ trait JsonNumberOptics {
)
}

final object JsonNumberOptics extends JsonNumberOptics {
Copy link
Author

Choose a reason for hiding this comment

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

stop scala3 compiler warning, also applies to other occurrences.

@inanme
Copy link
Author

inanme commented Jul 25, 2023

@zmccoy @zarthross please let me what you think, thanks

@zmccoy
Copy link
Member

zmccoy commented Jul 26, 2023

Hi @inanme at first glance we'll want keep the Scala versions at the Build level such as : https://github.com/circe/circe/blob/series/0.14.x/build.sbt#L13. That will keep 2.12,2.13, and 3 in github action workflow yaml.

You'll probably want to rebase as well as there were some merges from a few weeks ago that added in the sbt-circe-org plugin which helps make things a bit cleaner.

@inanme
Copy link
Author

inanme commented Jul 27, 2023

@zmccoy I welcome the SBT changes. With regards to my changes, I had to drop scala212 as monocle3 does not support/ dropped 212. Let me know if this is an issue. My main motivation for this PR to get monocle3 upgraded, scala3 nice to have.

@inanme
Copy link
Author

inanme commented Jul 27, 2023

You'll probably want to rebase

rebase done.

Copy link
Contributor

@GreyPlane GreyPlane left a comment

Choose a reason for hiding this comment

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

just few minor style issues, looks good!


final def applyDynamic(field: String)(index: Int): JsonTraversalPath = selectDynamic(field).index(index)

final def apply(i: Int): JsonTraversalPath = index(i)

final def index(i: Int): JsonTraversalPath =
JsonTraversalPath(json.composePrism(jsonArray).composeOptional(Index.index(i)))
JsonTraversalPath(json.andThen(jsonArray).andThen(Index.index(i)(Index.vectorIndex[Json])))
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be more stylish if written in Index.index[Vector[Json], Int, Json](i) IMO

Copy link
Author

Choose a reason for hiding this comment

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

thanks, agreed, 3e2abe3

Copy link
Author

Choose a reason for hiding this comment

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

also 844a837


final def filterByIndex(p: Int => Boolean): JsonTraversalPath =
JsonTraversalPath(arr.composeTraversal(FilterIndex.filterIndex(p)))
JsonTraversalPath(arr.andThen(FilterIndex.filterIndex(p)(FilterIndex.vectorFilterIndex[Json])))
Copy link
Contributor

Choose a reason for hiding this comment

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

same reason as above, written as FilterIndex.filterIndex[Vector[Json], Int, Json](p)

Copy link
Author

Choose a reason for hiding this comment

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

thanks, agreed, 3e2abe3

Copy link
Author

Choose a reason for hiding this comment

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

also 844a837

@zmccoy
Copy link
Member

zmccoy commented Aug 2, 2023

@inanme The dropping of 2.12 for monocle3 is not an issue in that case, I didn't know they dropped 2.12. We're going to want to think about branching strategy and version numbers expectations as this is bin incompat change.

@zmccoy
Copy link
Member

zmccoy commented Aug 2, 2023

The failures right now are expected as mima is showing that things are not bin compat. We can solve this once we figure out our branch / version strategy for this then :)

@GreyPlane
Copy link
Contributor

I didn't found any particular reason for Monocle to drop the support for Scala 2.12.x, Only because they're trying to reduce maintenance burden so decided to only support 2 major versions of Scala, I think we could ask them if it is possible to bring back it.

@zarthross
Copy link
Member

I didn't found any particular reason for Monocle to drop the support for Scala 2.12.x, Only because they're trying to reduce maintenance burden so decided to only support 2 major versions of Scala, I think we could ask them if it is possible to bring back it.

Sorry we weren't more clear, we can drop scala 2.12.x just fine. The binary incompatibility we are concerned with this the update to monocle 3. We can't just release this change as 0.14.2 because even if the changes in circe optics are compatible, we would be forcing a binary incompatible version of monocle on all consumers of circe optics. We do need this upgraded, but we need to decide between the options for how to go forward:

  1. We change the artifact name to circe-optics-monocle3 or similar and update the package names, so that we can release as version 0.14.2.
  • Pros: We can support monocle2 for longer
  • Cons: Confusion of artifact name change
  • Questions: what do we do when circe releases 0.15? Do we go back to circe-optics? Do we keep the long name
  1. We publish this as 0.15.0
  • Pros: We indicate the bin-incompat nature of the change, we don't have to rename any artifacts or packages
  • Cons: We create a LOT of confusion about the compatibility between circe-optics and Circe core.

@inanme
Copy link
Author

inanme commented Aug 3, 2023

Would this configuration work in circe ecosystem? I setup projects like this using sbt-projectmatrix.
0.14.2 -> (scala212 -> monocle2) -> circe-optics / /BAU
0.14.2 -> (scala213 -> monocle2) -> circe-optics / /BAU
0.14.2 -> (scala213 -> monocle3) -> circe-optics-monocle3 // new artefact
0.14.2 -> (scala3 -> monocle3) -> circe-optics // new artefact

@zarthross
Copy link
Member

@zmccoy and I discussed, and we think the best approach would be to use this naming scheme:

0.14.2 -> (scala212 -> monocle2) -> circe-optics
0.14.2 -> (scala213 -> monocle2) -> circe-optics
0.14.2 -> (scala213 -> monocle3) -> circe-optics-monocle_3 
0.14.2 -> (scala3   -> monocle3) -> circe-optics-monocle_3

We'll want to add something to the readme as well, to point users to this new artifact.

@GreyPlane How does this approach sound to you?

@GreyPlane
Copy link
Contributor

Personally, I'd like to say continuously support for circe-optics(monocle2) seems to unnecessary since Monocle itself dropped the support already, so release to next major version looks fine to me, and even Circe itself's major release holds bin comp, but it's more like a bonus since semantic version doesn't enforce that, so even at some point one of the Circe's associated project doesn't keep it, this behavior should be expected with some extent.

But I don't hold very strong opinion about this, especially if there is real world needs for using circe-optics with monocle 2, I'm OK with the new artifact.

@GreyPlane
Copy link
Contributor

@inanme I guess you have to exclude mima previous artifact for Scala 3 cross build as document says.

@zarthross and so, are we decided to release new artifact under a new name?

@QuitHub
Copy link

QuitHub commented Sep 12, 2023

It would be really great to have this available!

@zarthross
Copy link
Member

@GreyPlane, I think you have a persuasive argument. I'm ok with abandoning support for monocle 2, releasing a new version of circe-optics as 0.15 with monocle 3, and including in the readme a table with a note about the versioning and how it relates to circe-core. See https://github.com/circe/circe-generic-extras#versioning please use the same verbiage and structure for consistency.

@joan38
Copy link
Contributor

joan38 commented Sep 14, 2023

Hey, Thanks for the work here!
We are also blocked on this to upgrade to Scala 3.
Sounds like depending on the 2.13 package from Scala 3 is not working.

@GreyPlane
Copy link
Contributor

@inanme Hi buddy, I am currently un-employed, so I can pick this up as I have bit more time to focus. If you like me to look more into this issue.

@joan38
Copy link
Contributor

joan38 commented Sep 18, 2023

ResolveException: Error downloading io.circe:circe-optics_3:0.14.1

Why is it trying to resolve itself?

@joan38
Copy link
Contributor

joan38 commented Sep 19, 2023

So I understand the current build is not going through because of MiMa.

Is there anything we can do to move forward? Sounds like we have a sizable amount of people volunteering. Is the issue technical or political here?

We have projects we need to move forward on Scala 3 depending on circe-optics. Can a maintainer step forward?

@GreyPlane
Copy link
Contributor

@joan38 not any problem really, I think we have reach the consensus here and there isn't any hard technical issue, just have to finish the work.
I guess it's ok to pick this work into new branch and submit a new PR with respect to the original, you'd like to do it by yourself? or i can do it as i said before.

@zarthross
Copy link
Member

Thanks for the work on this.

@zarthross zarthross closed this Sep 20, 2023
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.

6 participants