Skip to content

Conversation

@gene-telligent
Copy link
Contributor

Noticed this while trying to write files (non dynamically) with a custom FileNaming.

Expected behavior: If an output directory is specified with a non-dynamic file write and a custom FileNaming, the generated filename should be resolved against the output directory.

Actual behavior: The output directory is ignored and the result of the FileNaming call becomes the full file path.

No JIRA ticket again because it looks like a quick code fix. @jkff once again can you take a quick look and approve?


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

  • [N/A] 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.
  • [N/A] 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 mvn clean verify 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.

Copy link
Contributor

@jkff jkff left a comment

Choose a reason for hiding this comment

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

Thank you! Any chance you could add a test for this? I think it should be easy to extract the part that resolves fileNamingFn into a utility function, and add this case as a unit test.

@gene-telligent
Copy link
Contributor Author

Yep, no problem! Pulled the code into its own function and added some tests for it. Let me know if you have any further feedback!

Copy link
Contributor

@jkff jkff left a comment

Choose a reason for hiding this comment

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

Thanks! Will dedupe the test code a bit and merge.

@asfgit asfgit closed this in 69e9ac8 Feb 22, 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.

2 participants