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

Python API name changes #339

Merged
merged 8 commits into from
Mar 31, 2023
Merged

Python API name changes #339

merged 8 commits into from
Mar 31, 2023

Conversation

ebezzi
Copy link
Member

@ebezzi ebezzi commented Mar 31, 2023

No description provided.

bkmartinjr
bkmartinjr reviewed Mar 31, 2023
View reviewed changes
.pre-commit-config.yaml Outdated
@@ -10,8 +10,8 @@ repos:
args: ["--config", "./api/python/notebooks/pyproject.toml"]
- id: black
name: black-cell-census
Copy link
Contributor

Choose a reason for hiding this comment

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

should these names change as well for clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most likely. Thanks for the catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most likely. Thanks for the catch.

@ebezzi ebezzi changed the title Python api name changes Python API name changes Mar 31, 2023
bkmartinjr
bkmartinjr reviewed Mar 31, 2023
View reviewed changes
api/python/cellxgene_census/release_process.md Outdated
@@ -1,8 +1,8 @@
# cell_census Python package release process
# cellxgene-census Python package release process
Copy link
Contributor

Choose a reason for hiding this comment

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

underscore or dash? We seem to mostly use underscore....

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, the only thing that should use the dash is the repo name. Python packages get aliased, so we might as well use the "import" name, which is underscored

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good

bkmartinjr
bkmartinjr suggested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

No major concerns. The only thing thing I'd like to resolve is the use of dash vs underscore in the package name when it is referenced outside of an import statement. Suggest we just use underscore for consistency.

@ebezzi ebezzi requested a review from bkmartinjr March 31, 2023 18:24
bkmartinjr
bkmartinjr approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

LGTM

@ebezzi ebezzi merged commit 59ccf5c into main Mar 31, 2023
@ebezzi ebezzi deleted the ebezzi/python-api-name-changes branch March 31, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkmartinjr bkmartinjr bkmartinjr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ebezzi @bkmartinjr