-
Notifications
You must be signed in to change notification settings - Fork 871
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
Rename _transcoding
module, deprecate old constant
#3826
Conversation
ef50c6f
to
18598ee
Compare
18598ee
to
c3fd791
Compare
Signed-off-by: Deepyaman Datta <[email protected]>
c3fd791
to
1aa7434
Compare
I do think that the private module is leaky, the methods are private but then we expose The only thing absolutely blocking here is the circular dependency, this PR is an alternative to it. @deepyaman make a point that it shouldn't be just 0.19 backward compatible, would we should continue to expose For private/public, there are different ways.
|
I agree with the point of moving IMO, from the implementational point of view, keeping transcoding logic in a separate file looks better rather than moving it back to the pipeline and fixing circular dependency. |
Since there is some time to discuss this now—what will be the recommended/supported way to import
|
Do both for 0.19, keep 1 only starting from 0.20 going forward. Similar to https://github.com/kedro-org/kedro/pull/1837/files#diff-c9b9e2fdad60057c915a16d9caf8c11637750cd6094585b4ad2f583df619ddac This helps to avoid codebase diverge from |
This reverts commit 1aa7434. Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
8c25bb2
to
8b70a57
Compare
pipeline
_transcoding
module, deprecate old constant
_transcoding
module, deprecate old constant_transcoding
module, deprecate old constant
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @deepyaman, LGTM!
Approving the PR as it makes sense to make the transcoding
module public as discussed.
However, we might consider exposing methods to apply the transcoding instead of the constant. It will guarantee that users follow the same logic when applying it rather than implementing their own based on the constant.
FWIW I actually have found access to the constant most useful in plugin development. Maybe functions to apply transcoding, split on separator, and check if something is transcoded could work, but I think it's less a user need in my eyes. |
Description
This leaves
TRANSCODING_SEPARATOR
in a public module; there is no expectation that it would move to a private module.Discussion in #3826.
Developer notes
Kedro-Viz will be fine, but https://github.com/kedro-org/kedro-viz/blob/main/package/kedro_viz/integrations/kedro/hooks.py#L15-L17 could get dropped again, if so desired. (Nobody needs to be on Kedro 0.19.4, since there will be no functional differences between that and 0.19.5.)
Alternatives considered
It's possible to rename the new
_transcoding
module totranscoding
, but this avoids that and goes back to something very similar to what was there in 0.19.3 and below.Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file