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

[Bug]: WriteToFiles(output_fn=...) is seemingly unused #30009

Closed
1 of 16 tasks
dlwh opened this issue Jan 12, 2024 · 7 comments · Fixed by #32635
Closed
1 of 16 tasks

[Bug]: WriteToFiles(output_fn=...) is seemingly unused #30009

dlwh opened this issue Jan 12, 2024 · 7 comments · Fixed by #32635

Comments

@dlwh
Copy link

dlwh commented Jan 12, 2024

What happened?

There's a ctor argument output_fn to WriteToFiles that isn't used other than setting a member variable that is never read:

Issue Priority

Priority: 3 (minor)

Issue Components

  • Component: Python SDK
  • Component: Java SDK
  • Component: Go SDK
  • Component: Typescript SDK
  • Component: IO connector
  • Component: Beam YAML
  • Component: Beam examples
  • Component: Beam playground
  • Component: Beam katas
  • Component: Website
  • Component: Spark Runner
  • Component: Flink Runner
  • Component: Samza Runner
  • Component: Twister2 Runner
  • Component: Hazelcast Jet Runner
  • Component: Google Cloud Dataflow Runner
@tvalentyn
Copy link
Contributor

agreed, appears to be never used. We could clean it up and force keyword-only arguments after it. It could be a breaking change for some users, so could also not touch it and add a note that it's unused and leftover for backward compatibiltiy.

@ima-helikoptaaa
Copy link

.take-issue

@riteshghorse
Copy link
Contributor

@ima-attac-helikoptaaa are you still working on it?

@tsafacjo
Copy link

agreed, appears to be never used. We could clean it up and force keyword-only arguments after it. It could be a breaking change for some users, so could also not touch it and add a note that it's unused and leftover for backward compatibiltiy.

To sum up we shouldn't change it ?

@tvalentyn
Copy link
Contributor

we can still make the clarification in the code that it is unused. Removing is fine too as long as we force keyword-only args.

@hayk2377
Copy link

hayk2377 commented Oct 1, 2024

agreed, appears to be never used. We could clean it up and force keyword-only arguments after it. It could be a breaking change for some users, so could also not touch it and add a note that it's unused and leftover for backward compatibiltiy.

should we add a note on the docstring so that it is visible for the caller

@mohamedawnallah
Copy link
Contributor

mohamedawnallah commented Oct 2, 2024

.take-issue

EDIT:
I'm new here and eager to contribute. Since this issue has been stale for a while and no pull request has been submitted, I would like to take the lead on it! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment