Skip to content

Conversation

xuwei-k
Copy link
Contributor

@xuwei-k xuwei-k commented Sep 13, 2025

No description provided.

@sjrd
Copy link
Member

sjrd commented Sep 13, 2025

For Scala 3 support, I would prefer a single PR with multiple commits: one for each kind of required change in the code, and finally one that adds building with Scala 3. Otherwise we can't really test whether the changes actually work.

@xuwei-k xuwei-k marked this pull request as draft September 13, 2025 09:48
@xuwei-k xuwei-k mentioned this pull request Sep 25, 2025
@xuwei-k xuwei-k changed the title fix scala 3 warning. remove redundant final add Scala 3 build Sep 25, 2025
}

@tailrec
private def waitOnMessageLoop(deadline: Deadline): String = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

final object RunConfig {
object RunConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[error] 122 |final object RunConfig {
[error]     |^^^^^
[error]     |Modifier final is redundant for this definition

@xuwei-k xuwei-k marked this pull request as ready for review September 25, 2025 01:55
@sjrd
Copy link
Member

sjrd commented Sep 25, 2025

The code changes look good. I only have nits on the commit messages:

  • Please directly include your self-review comments in the relevant commit messages. They justify the existence of those commits, so they are important.
  • While you're at it, ideally follow the commit message style we use, which starts with an uppercase (e.g., "Add Scala 3 build" instead of "add").

```
[error] 122 |final object RunConfig {
[error]     |^^^^^
[error]     |Modifier final is redundant for this definition
```
@xuwei-k xuwei-k changed the title add Scala 3 build Add Scala 3 build Oct 12, 2025
@sjrd sjrd merged commit 5f7e20a into scala-js:main Oct 13, 2025
14 checks passed
@xuwei-k xuwei-k deleted the final-object branch October 13, 2025 11:14
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