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

Casing Consistency: Rename TensorFlowModelDataset to ...DataSet #2494

Closed
BrianCechmanek opened this issue Apr 4, 2023 · 7 comments
Closed
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@BrianCechmanek
Copy link

Description

TensorFlowModelDataset is the only included dataset not following the "DataSet" casing consistency.

Context

Maintaining consistency assists with mental overhead.

Possible Implementation

I believe the fix is just a refactor in the 7 instances across the following two files:

  • ./extras/datasets/tensorflow/tensorflow_model_dataset.py
  • ./extras/datasets/tensorflow/__init__.py:__all__

I am happy to submit a PR of the refactor, if appropriate.

Possible Alternatives

It may have been intentional to break convention for this DataSet. Searching open and closed Issues, I found no reference.

NB : I believe I followed the contributing guide correctly. Please let me know if not - first ever OS Issue submission :)

@BrianCechmanek BrianCechmanek added the Issue: Feature Request New feature or improvement to existing feature label Apr 4, 2023
@deepyaman
Copy link
Member

@BrianCechmanek Great catch! It's not intentional (AFAIK), just an oversight. :) You're welcome to make a PR, but please do so on https://github.com/kedro-org/kedro-plugins instead (the new home from datasets).

@deepyaman
Copy link
Member

It would also be nice to make everything SomethingSomethingDataset in the future, to be most consistent (see #533), but that would be outside the scope of this PR (and probably should be aligned with the broader team).

@BrianCechmanek
Copy link
Author

Tubular!

  1. Will get on this (PR to plugins, per your comment)
  2. Is there any consideration like a deprecation warning for changing Dataset -> DataSet ?

@deepyaman
Copy link
Member

  1. Is there any consideration like a deprecation warning for changing Dataset -> DataSet ?

I don't think it's necessary for datasets, but let me confirm...

@astrojuanlu
Copy link
Member

See #2129

@BrianCechmanek
Copy link
Author

Thanks @astrojuanlu - sorry I missed that.

Sounds like that's a much bigger task (and probably above my head). I think this can be safely closed, then.

@BrianCechmanek BrianCechmanek closed this as not planned Won't fix, can't repro, duplicate, stale Apr 4, 2023
@deepyaman
Copy link
Member

@BrianCechmanek I think it may be fine to go ahead with this, since the massive change will probably have to wait until 0.19 release, while this could help improve consistency in the shorter term. But maybe we can just wait for a confirmation on this; don't want you to do work unnecessarily. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
None yet
Development

No branches or pull requests

3 participants