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.

Sign up for GitHub

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

Jump to bottom

Rename addr to address in non-public API. #23432

Merged
merged 2 commits into from
Aug 8, 2017
Merged

Rename addr to address in non-public API. #23432

merged 2 commits into from
Aug 8, 2017

Conversation

jiminhsieh
Copy link
Contributor

@jiminhsieh jiminhsieh commented Jul 26, 2017 •
edited
Loading

Issue - #21874
I will use another commit to change attributes from addr to address in Java. I found previous PR, #21885, not only simply change addr to address but also methods and attributes contained addr in the middle. For instance, getAddrString to getAddressString. Is it fine for the Akka-Team? Thanks!

@akka-ci
Copy link

akka-ci commented Jul 26, 2017

Can one of the repo owners verify this patch?

johanandren
johanandren approved these changes Jul 26, 2017
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Not a huge improvement, but LGTM, I guess.

@johanandren
Copy link
Member

OK TO TEST

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jul 26, 2017
@akka-ci
Copy link

akka-ci commented Jul 26, 2017

Test FAILed.

@johanandren
Copy link
Member

Failure is formatting, the code is auto formatted when compiled, so please compile all changed code locally and then commit the changes and push that.

@jiminhsieh
Copy link
Contributor Author

@johanandren Really sorry for that stupid mistake. I only used IntelliJ in local without sbt. By the way, can I verify broke public API in the local with sbt test? Thanks!

@raboof
Copy link
Member

raboof commented Jul 26, 2017

@jiminhsieh for checking binary compatibility have a look at https://github.com/akka/akka/blob/master/CONTRIBUTING.md#binary-compatibility

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Jul 27, 2017
@akka-ci
Copy link

akka-ci commented Jul 27, 2017

Test FAILed.

@johanandren
Copy link
Member

Seems this caused a few test failures now, plus binary compatibility errors.

See https://jenkins.akka.io:8498/job/pr-validator-per-commit-jenkins/9885/ for details.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Aug 1, 2017
@akka-ci
Copy link

akka-ci commented Aug 1, 2017

Test PASSed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Aug 1, 2017
johanandren
johanandren reviewed Aug 1, 2017
View reviewed changes
akka-actor/src/main/scala/akka/actor/ActorPath.scala Outdated
@@ -272,11 +272,11 @@ final case class RootActorPath(address: Address, name: String = "/") extends Act

override val toSerializationFormat: String = toString

override def toStringWithAddress(addr: Address): String =
override def toStringWithAddress(newAddress: Address): String =
Copy link
Member

Choose a reason for hiding this comment

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

These are actually not source compatible, since the parameter names in Scala are a part of the API (because you could use named params, not likely that anyone does for this but we cannot know nobody did a toStringWithAddress(addr = something) which will not work anymore with this change.

Strange that MiMa didn't complain about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I got it. Thanks for you explanation! : ) I will rollback this soon. BTW, maybe it would be a good idea to send an issue to MiMa? They would know this situation.

Copy link
Member

Choose a reason for hiding this comment

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

Ping @ktoso or @2m is this perhaps source incompatible but MiMa only cares about binary incompatibilities?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is breaking only for scala in that specific use case.
Scala has tools to deal with that though - the def method(@deprecatedName("addr") newOne: Address) syntax can make that work, though I'm unsure we care enough about this name here to make the change - let's not change it in method params perhaps?

Copy link
Member

Choose a reason for hiding this comment

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

Link to docs https://www.scala-lang.org/api/current/scala/deprecatedName.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does test code need to be consider compatibility? Normally, it would be to fine to change public methods or public attributes in test code. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Tests as well as the testkit (which is public API but exempt from these rules) are fine to change :)

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Aug 1, 2017
@akka-ci
Copy link

akka-ci commented Aug 1, 2017

Test PASSed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Aug 1, 2017
@akka-ci
Copy link

akka-ci commented Aug 1, 2017

Test PASSed.

@jiminhsieh
Copy link
Contributor Author

jiminhsieh commented Aug 2, 2017 •
edited
Loading

I will review the compatibility of each file again. Since there are some use cases MiMa could not detect. For example - https://github.com/akka/akka/pull/23432/files/f6d54455d28c0e5e1bae7e82e056bcfea98065da#r130586385.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Aug 2, 2017
@akka-ci
Copy link

akka-ci commented Aug 2, 2017

Test PASSed.

@jiminhsieh
Rename addr to full phrase.
7c86dd2
@jiminhsieh
Rollback to original naming.
bf700ab 
It will break source compatibility. For example, toStringWithAddress(addr = something).
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Aug 3, 2017
@akka-ci
Copy link

akka-ci commented Aug 3, 2017

Test PASSed.

johanandren
johanandren approved these changes Aug 3, 2017
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM

patriknw
patriknw approved these changes Aug 7, 2017
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM

@johanandren johanandren merged commit f623d10 into akka:master Aug 8, 2017
@jiminhsieh jiminhsieh deleted the issue/wip-21874 branch August 8, 2017 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso left review comments

@patriknw patriknw patriknw approved these changes

@johanandren johanandren johanandren approved these changes

Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jiminhsieh @akka-ci @johanandren @raboof @ktoso @patriknw