Skip to content

Conversation

@udim
Copy link
Member

@udim udim commented Apr 6, 2018

Also fixes outdated references to lint_py2 and lint_py3.

@udim
Copy link
Member Author

udim commented Apr 6, 2018

R: @melap

To Check for lint errors locally, run the following command.

$ mvn clean verify -pl sdks/python
$ ../../gradlew lint
Copy link
Member

Choose a reason for hiding this comment

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

Make it clear that ../.. is here because we assume it is being run from the sdks/python directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@aaltay
Copy link
Member

aaltay commented Apr 6, 2018

Dead link error is

- ./.testcontent/contribute/work-in-progress/index.html
  *  External link https://github.com/apache/beam/tree/go-sdk failed: 404 No error

@melap does it need to be fixed in this PR?

@melap
Copy link

melap commented Apr 6, 2018

retest this please

@youngoli
Copy link

youngoli commented Apr 6, 2018

So I didn't realize you were working on this. I made some overlapping changes in my PR #414, so one of us will probably need to merge the changes.

@udim
Copy link
Member Author

udim commented Apr 6, 2018

Sorry for not coordinating with you @youngoli, let's merge #414 first and I'll deal with the conflicts in mine.
BTW, what do you think of my suggestions for running tests and lint? I don't believe that running clean first is necessary, but I might be wrong.

$ mvn clean verify
Run this command to test for lint errors.

$ ./gradlew lint
Copy link

Choose a reason for hiding this comment

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

I don't think this needs to be mentioned here since the lint task only lints the Python code right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

git repository.

For contributions to the Java code, run unit tests locally via Maven.
$ ./gradlew test
Copy link

Choose a reason for hiding this comment

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

Perhaps adding some instructions for calling unit tests on certain languages may be useful. This is slightly different than the precommit info down below because it is only unit tests.

Examples:

$ ./gradlew sdks:java:test
$ ./gradlew sdks:python:test
$ ./gradlew sdks:go:test

Also may be worth using the "check" task instead of "test". According to the gradle docs:

It is common for all verification tasks, including tests and linting, to be executed using the check task. (Source)

I don't know if Beam is doing this at the moment, but I feel like this area of the code should mention using check for more intense checks (i.e. for replacing mvn verify), while test is better for basic unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced with check, which includes lint (on some SDKs).
I replaced the precommit tasks below with *:check, for language SDKs that had them (I don't know if there's a Java check task for all parts of the Java SDK).

@youngoli
Copy link

youngoli commented Apr 6, 2018

@udim You're probably right about the ./gradlew clean being unnecessary. I only had it there because it was mentioned in the original code with mvn clean verify.


You can also limit tests to certain language SDKs.

$ ./gradlew javaPreCommit
Copy link
Member

Choose a reason for hiding this comment

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

These precommits typically need more like a GCP account since they run wordcount against Dataflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so what is the suggested gradle task to run java tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed javaPreCommit

@udim udim force-pushed the asf-site branch 2 times, most recently from 9a6abc5 to 93de5e8 Compare April 10, 2018 22:10
@aaltay
Copy link
Member

aaltay commented Apr 12, 2018

@udim Could you rebase and update this PR. Is this still relevant after #414?

Also update Python SDK instructions and reorganize them a bit.
@udim
Copy link
Member Author

udim commented Apr 12, 2018

Rebased.

@aaltay
Copy link
Member

aaltay commented Apr 12, 2018

Thank you @udim. LGTM.

@aaltay
Copy link
Member

aaltay commented Apr 12, 2018

@asfgit merge

asfgit pushed a commit that referenced this pull request Apr 12, 2018
@asfgit asfgit merged commit 29600fd into apache:asf-site Apr 12, 2018
@asfgit
Copy link

asfgit commented Apr 12, 2018

Error: Adding files from repository preparation failed. Please try again.

@aaltay
Copy link
Member

aaltay commented Apr 12, 2018

@melap I see the merged commit in the repository (https://gitbox.apache.org/repos/asf?p=beam-site.git;a=log;h=refs/heads/asf-site). Do we need to take action to resolve the latest error message from the merge bot?

@melap
Copy link

melap commented Apr 12, 2018

good question -- it looks like the changes are up on the website too, so I am unsure what that error means. @jasonkuster or @alanmyrvold should we just ignore this error?

robertwb pushed a commit to robertwb/incubator-beam that referenced this pull request Jun 5, 2018
robertwb pushed a commit to robertwb/incubator-beam that referenced this pull request Jun 5, 2018
melap pushed a commit to apache/beam that referenced this pull request Jun 20, 2018
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.

6 participants