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

Deprecate (mark for future removal) get_pkg_version from the public API #3789

Closed
noklam opened this issue Apr 8, 2024 · 6 comments · Fixed by #3947
Closed

Deprecate (mark for future removal) get_pkg_version from the public API #3789

noklam opened this issue Apr 8, 2024 · 6 comments · Fixed by #3947
Assignees

Comments

@noklam
Copy link
Contributor

noklam commented Apr 8, 2024

Context

          @merelcht It didn't change by #3742. 

def get_pkg_version(reqs_path: (str | Path), package_name: str) -> str:
"""Get package version from requirements.txt.
Args:
reqs_path: Path to requirements.txt file.
package_name: Package to search for.
Returns:
Package and its version as specified in requirements.txt.
Raises:
KedroCliError: If the file specified in ``reqs_path`` does not exist
or ``package_name`` was not found in that file.
"""
reqs_path = Path(reqs_path).absolute()

I just check again, this function is not used anywhere in framework, but only tests. (last change is 4 years ago). Maybe at some points it was used for kedro build-reqs? I suggest we deprecate this function since it's a public function and move it to test.

Originally posted by @noklam in #2912 (comment)

Remove the function from public API and move it to test only.

@noklam
Copy link
Contributor Author

noklam commented Apr 11, 2024

One question here, we tend to do the breaking change later in develop to avoid the code diverge too much. At the same time should we add the deprecated warnings earlier in 0.19 to inform user?

This is not important for this particular issue, because I think no one use it (hopefully), but it is a valid question for what should we do in general?

@astrojuanlu
Copy link
Member

should we add the deprecated warnings earlier in 0.19 to inform user?

Yes

@noklam
Copy link
Contributor Author

noklam commented Apr 11, 2024 via email

@astrojuanlu
Copy link
Member

I'd say let's rename this ticket to "Deprecate get_pkg_version" and groom it

@astrojuanlu astrojuanlu changed the title Remove get_pkg_version from the public API Deprecate (mark for future removal) get_pkg_version from the public API Apr 12, 2024
@noklam
Copy link
Contributor Author

noklam commented Apr 22, 2024

Actually the tests are obsoleted and we should remove it altogether class TestCliUtils in test_cli.py.

@astrojuanlu
Copy link
Member

Funny bit of insight found in the source code of kedro-mlflow:

            # if it is a local relative path, make it absolute
            # .resolve() does not work well on windows
            # .absolute is undocumented and have known bugs
            # Path.cwd() / uri is the recommend way by core developpers.
            # See : https://discuss.python.org/t/pathlib-absolute-vs-resolve/2573/6

Not sure how up to date it is though (the thread is from 2019).

@merelcht merelcht moved this to To Do in Kedro Framework Jun 5, 2024
@merelcht merelcht moved this from To Do to In Review in Kedro Framework Jun 11, 2024
@merelcht merelcht self-assigned this Jun 11, 2024
@github-project-automation github-project-automation bot moved this from In Review to Done in Kedro Framework Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants