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.

Sign up for GitHub

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

Jump to bottom

Type hints and doc strings: conda.cli.main_info #13445

Merged
merged 18 commits into from
Jan 5, 2024
Merged

Type hints and doc strings: conda.cli.main_info #13445

merged 18 commits into from
Jan 5, 2024

Conversation

samhaese
Copy link
Contributor

Description

This PR is part of an effort to add type hints and doc strings (#13437 ). This PR only modified conda.cli.main_info by adding type hints and rst-style doc strings, with one exception (changed the name of the argument for dump_record from pkg to prec).

This is my first PR for this effort, so there were a few things that I'm not sure about. For example, how to check API documentation rendering as suggested in the issue. Advice and feedback appreciated :)

Miscellaneous notes

Going through the code for type hints and descriptions, I came across a few things that might deserve attention.

  • Should the get_user_site function be updated with platformdirs? Right now it returns possibly misleading information.
  • Several of the conda info usages have no --json support

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@samhaese samhaese requested a review from a team as a code owner December 19, 2023 02:06
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Dec 19, 2023
kenodegard
kenodegard requested changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

This is a great start! Only suggestion I have is to make the return types more specific.

Regarding reviewing the rendered docs, these are auto generated as part of PR checks/GH Actions (see the ReadTheDocs check below).

conda/cli/main_info.py Outdated Show resolved Hide resolved
conda/cli/main_info.py Outdated Show resolved Hide resolved
conda/cli/main_info.py Outdated Show resolved Hide resolved
conda/cli/main_info.py Outdated Show resolved Hide resolved
conda/cli/main_info.py Outdated Show resolved Hide resolved
conda/cli/main_info.py Outdated Show resolved Hide resolved
samhaese and others added 3 commits December 18, 2023 20:29
@samhaese
Copy link
Contributor Author

pre-commit.ci autofix

@travishathaway
Copy link
Contributor

@samhaese,

Just FYI, we build a documentation preview for each pull request:

Bildschirmfoto 2023-12-19 um 06 59 06

Click the "details" link to see it.

travishathaway
travishathaway previously approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@travishathaway travishathaway left a comment

Choose a reason for hiding this comment

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

I think this looks good. Waiting for @kenodegard to approve too before merging.

Thank you!

@kenodegard
Copy link
Contributor

blocked by unrelated test failures, waiting on #13449

@samhaese samhaese mentioned this pull request Dec 20, 2023
Open
@kenodegard kenodegard enabled auto-merge (squash) December 22, 2023 18:45
jezdez
jezdez requested changes Dec 22, 2023
View reviewed changes
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

Specifying the return type again in the docstring is a duplication of information and not needed because sphinx-autoapi supports autodoc's ability to automatically derive it from the object's typehints!

Thank you for updating the docstrings with blurbs of course and adding the typehints, but could you please remove the :returns: statements?

@kenodegard
Copy link
Contributor

I think we still want the :returns:, it's the :rtype: which we can forego with sphinx-autoapi

@samhaese
Copy link
Contributor Author

Specifying the return type again in the docstring is a duplication of information and not needed because sphinx-autoapi supports autodoc's ability to automatically derive it from the object's typehints!

Thank you for updating the docstrings with blurbs of course and adding the typehints, but could you please remove the :returns: statements?

Please correct me if I'm wrong but I believe the :returns: directive only supplies descriptive language for sphinx autodoc, not the return type. Do you still recommend removal?

@samhaese samhaese requested a review from jezdez December 26, 2023 19:51
@samhaese samhaese mentioned this pull request Dec 27, 2023
Merged
3 tasks
kenodegard
kenodegard requested changes Jan 4, 2024
View reviewed changes
conda/cli/main_info.py Outdated Show resolved Hide resolved
news/13437-add-type-hints-main_info Outdated Show resolved Hide resolved
kenodegard
kenodegard reviewed Jan 4, 2024
View reviewed changes
conda/cli/main_info.py Outdated Show resolved Hide resolved
samhaese and others added 2 commits January 4, 2024 08:54
@samhaese @kenodegard
Added TYPE_CHECKING condition to import
358bc36 
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
@samhaese @kenodegard
Minor text change in news blurb
15069b0 
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
kenodegard
kenodegard reviewed Jan 4, 2024
View reviewed changes
conda/cli/main_info.py Outdated Show resolved Hide resolved
samhaese and others added 2 commits January 4, 2024 08:56
@samhaese @kenodegard
Removed :returns: from print_package_info
a3850c3 
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
@samhaese @kenodegard
Deprecated get_info_dict argument
174ca82 
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
@kenodegard
Copy link
Contributor

Specifying the return type again in the docstring is a duplication of information and not needed because sphinx-autoapi supports autodoc's ability to automatically derive it from the object's typehints!
Thank you for updating the docstrings with blurbs of course and adding the typehints, but could you please remove the :returns: statements?

Please correct me if I'm wrong but I believe the :returns: directive only supplies descriptive language for sphinx autodoc, not the return type. Do you still recommend removal?

@samhaese you are correct, no need to remove :returns:

@kenodegard
Copy link
Contributor

pre-commit.ci autofix

@jezdez jezdez merged commit 2cf920a into conda:main Jan 5, 2024
73 checks passed
@samhaese samhaese deleted the main_info-type-hints branch January 6, 2024 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jezdez jezdez jezdez approved these changes

@kenodegard kenodegard kenodegard approved these changes

@travishathaway travishathaway travishathaway left review comments

Assignees
No one assigned
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
🔎 Review
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@samhaese @travishathaway @kenodegard @jezdez @conda-bot