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

JVM: Don't use deprecated Class#newInstance() #40

Merged
merged 1 commit into from Apr 3, 2021

Conversation

sideeffffect
Copy link
Contributor

Copy link
Contributor

@gzm0 gzm0 left a comment

Choose a reason for hiding this comment

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

Only the diff issues

@sideeffffect
Copy link
Contributor Author

@gzm0 those diffs are there so that it merges well with the original PR. It seemed easier this way. Or do you think it would be better to remove them anyway?

@sideeffffect sideeffffect requested a review from gzm0 April 1, 2021 19:07
@gzm0
Copy link
Contributor

gzm0 commented Apr 3, 2021

Or do you think it would be better to remove them anyway?

Yes I think removing them would be better. We should trade the other PR if there are any conflicts.

The reason for this is that we want a single PR to change one thing only (mich easier to understand the history that way).

@sideeffffect
Copy link
Contributor Author

Sure thing! Done ✔️

@gzm0
Copy link
Contributor

gzm0 commented Apr 3, 2021

LGTM. We can merge this after you squash all the commits. (unless @sjrd has further comments).

@sideeffffect
Copy link
Contributor Author

@gzm0 I would be more than happy to do the squashing myself, but if you don't mind me asking, why don't you just do the Squash-merge of this PR yourself? GitHub makes it a trivial one-click-button operation 😸

@gzm0
Copy link
Contributor

gzm0 commented Apr 3, 2021

Good question :) A bunch of reasons: On one hand habit from pre-GitHub support times. However, more importantly, Scala.js (and due to its history, this repo) uses --no-ff merges. So every merge creates an individual merge commit, even if the branch could be fast forwarded. Or in other words: The master branch only contains merge commits and special (release) commits directly. GitHub does not support this flavor of squash and merge.

Why do we do this: It allows us to have PRs that have multiple meaningful commits (which we use quite extensively in Scala.js core, for example: scala-js/scala-js#4450).

@sideeffffect
Copy link
Contributor Author

IMHO this PR is a good example where squash merge from GitHub UI would make things easier for everybody, but never mind, it's squashed now 😉

@gzm0 gzm0 merged commit 6303234 into portable-scala:master Apr 3, 2021
@sideeffffect sideeffffect deleted the getDeclaredConstructor branch April 3, 2021 21:16
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.

None yet

3 participants