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

[BEAM-2163] Remove the dependency on examples from ptransform_test #5199

Merged
merged 2 commits into from Apr 30, 2018

Conversation

jglezt
Copy link
Contributor

@jglezt jglezt commented Apr 21, 2018

The test_read_metrics function no longer depends on the snippets file. The CountingSource class is contained in io.utils which forms part of the library.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand:
    • What the pull request does
    • Why it does it
    • How it does it
    • Why this approach
  • Each commit in the pull request should have a meaningful subject line and body.
  • Run ./gradlew build to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@pabloem
Copy link
Member

pabloem commented Apr 23, 2018

Hello Javier.
Since CountingSource might be used in other cases (e.g. other tests), could you add it to beam/sdks/python/apache_beam/io/ instead?

@jglezt
Copy link
Contributor Author

jglezt commented Apr 24, 2018

It makes sense to put the class on the iobase.py or is preferable to create a single file for this class?

@pabloem
Copy link
Member

pabloem commented Apr 24, 2018

I was trying to find a file that it's appropriate for. I'm thinking that a new file should be fine for it.

@jglezt jglezt force-pushed the issue_2163 branch 2 times, most recently from 56b14ba to cd13b0a Compare April 25, 2018 00:47
@jglezt
Copy link
Contributor Author

jglezt commented Apr 25, 2018

Hi Pablo
I put the class on a general snippets file inside the io folder. I put named the file snippets in case any other function or class that is required on the io folder, but does not makes sense under any other file.

@pabloem
Copy link
Member

pabloem commented Apr 26, 2018

Hi Javier!
I'm sorry about the back-and-forth. Could you rename the file to 'utils' instead of 'snippets', and remove the [START model_custom_source_new_source] and [END model_custom_source_new_source]?
Thanks!

@jglezt
Copy link
Contributor Author

jglezt commented Apr 27, 2018

Hi Pablo
Thanks for the feedback : )
Changed file name and removed comments as suggested.

@pabloem pabloem merged commit 9d75d06 into apache:master Apr 30, 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.

None yet

2 participants