-
Notifications
You must be signed in to change notification settings - Fork 22
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
Migrate to Selenium 4, add Scala 3 #131
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for taking the time to open this PR.
The overall approach looks good to me. I have left some specific comments about the interface.
A general note: We try to keep changes as isolated as possible, so ideally, we do the Scala and sbt upgrades in different PRs.
Regarding the Scala upgrade: IIUC for a library it is better to stay on the lowest possible version to not force upgrades downstream, so maybe we shouldn't do them? (dropping Scala 2.11 is of course OK). Regarding Scala 3: I have to defer to @sjrd
Lastly, there seem to be some unrelated formatting stylistic changes. We also try to avoid these as much as possible to remove noise in the diffs. If you like, I can point them out individually.
seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumJSEnv.scala
Outdated
Show resolved
Hide resolved
seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/package.scala
Outdated
Show resolved
Hide resolved
Fair enough, I'll split.
As far as I understand, as long as it's under the same minor series (3.3, 2.13, 2.12) it should be fine to use a more recent version, even if users don't
That's alright, I'll go through the diff to make these unnecessary changes minimal. I did run scalastyle locally, it just didn't do anything :) Thanks for your comments, I'll update the PR soon! |
I couldn't keep the sbt upgrade separate as it's a prerequisite for me to literally launch sbt in the project in the first place. I could move it to a separate PR and make that the base of this one, but I'm not sure that's adding a lot of value in a project with such low traffic. Instead, I've made it a separate commit (first one, so that the others could be rolled back independently if needed), does that work for you? Also, new change: I didn't notice it before, but Selenium now requires JDK 11 (compiles to the classfile version 55.0), so I updated the PR description and the CI setup. |
Also updated the README. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long silence on this one. It slipped through the cracks.
There is only one nit, otherwise this LGTM to merge.
I've noticed that the CI didn't run, but I don't seem to have a button to approve it.
@sjrd do you have one? From the actions tab, it looks like you approved this last time.
seleniumJSEnv/src/main/scala/org/scalajs/jsenv/selenium/SeleniumJSEnv.scala
Outdated
Show resolved
Hide resolved
Huh, no, I don't have a button to run the CI. Perhaps it was too long ago? A force-push might make it appear. |
try now please :) |
The CI did run, but it has failures. |
DriverFactory
from Selenium isn't a thing so we make our ownDriverFactory
has to be provided explicitly by the user.