[BEAM-2230] ApiSurface Refactoring

[evindj]

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

• Make sure the PR title is formatted like:
  [BEAM-<Jira issue #>] Description of pull request
• Make sure tests pass via mvn clean verify.
• Replace <Jira issue #> in the title with the actual Jira issue
  number, if there is one.
• If this contribution is large, please file an Apache
  Individual Contributor License Agreement.

---

[coveralls]

Coverage increased (+0.04%) to 71.011% when pulling 25c6a62 on evindj:BEAM-2230 into 9f904dc on apache:master.

[evindj]

@kennknowles would you please review and let me know if I need to make some changes?

[kennknowles]

I don't think we need to make this abstract and have subclasses extend ApiSurface. Really, we just need to build values of type ApiSurface. Something like this: directRunnerApiSurface() { return <build the API surface>; }.

[evindj]

I evaluated writing custom methods in ApiSurface but decided not to go that route because some of the ApiSurface test implementation made calls like getClass().getPackage() which I think make them couple to the namespace where they are defined. Also, I think it is good to provide each package a way to build their ApiSurface they way they want through the buildApiSurface() Method.

[kennknowles]

I mean this:

package org.apache.beam.sdk.util;

public class ApiSurface { ... just like now ... }

And separately

class DirectRunnerApiSurfaceTest {
  private static ApiSurface directRunnerApiSurface() { ... }

  @Test
  public void testApiSurface() {
    assertThat(directRunnerApiSurface(), containsOnlyClassFrom(....));
  }
}

[evindj]

I see what you are saying now ApiSurface is not meant to be defined by modules just to verify the set of entities they expose. will make the change accordingly. Thanks for the review.

[kennknowles]

Why is it not static now? And for all of the methods below, too.

[evindj]

This is a side effect of making the class abstract, since it was constructing an object.

[coveralls]

Coverage decreased (-0.01%) to 70.598% when pulling e70a64d on evindj:BEAM-2230 into 1d9160f on apache:master.

[evindj]

@kennknowles modified this as per your comments.

[evindj]

Please @kennknowles Still waiting for your feedback on this.

[kennknowles]

R: @kennknowles

I was just on some vacation, but will be doing lots of reviews now that I am back. Soon.

[evindj]

ping @kennknowles

[kennknowles]

In fact, the way the ApiSurface tests have been made to scan the classpath is pretty fundamentally broken. I think probably the best thing to do to avoid blocking peoples' work is to remove the tests.

(your change is looking good, but the basis is just not right, sorry for the time you spent here!)

[evindj]

@kennknowles no problem, I am going to look into removing them if that has not been taken care of yet.

[kennknowles]

We have turned on autoformatting of the codebase, which causes small conflicts across the board. You can probably safely rebase and just keep your changes. Like this:

$ git rebase
... see some conflicts
$ git diff
... confirmed that the conflicts are just autoformatting
... so we can just keep our changes are do our own autoformat
$ git checkout --theirs --
$ git add -u
$ git rebase --continue
$ ./gradlew spotlessJavaApply

Please ping me if you run into any difficulty.

[stale]

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

[stale]

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.