Skip to content
This repository was archived by the owner on Oct 21, 2022. It is now read-only.

Python compatibility guidelines#169

Merged
liyanhui1228 merged 29 commits into
GoogleCloudPlatform:masterfrom
liyanhui1228:guidelines
Nov 15, 2018
Merged

Python compatibility guidelines#169
liyanhui1228 merged 29 commits into
GoogleCloudPlatform:masterfrom
liyanhui1228:guidelines

Conversation

@liyanhui1228
Copy link
Copy Markdown
Member

@liyanhui1228 liyanhui1228 commented Oct 23, 2018

Closes #113

Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst
ylil93 and others added 3 commits October 24, 2018 10:34
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Comment thread best_practices/README.rst
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst Outdated
Comment thread best_practices/README.rst
Comment thread best_practices/README.rst Outdated

- It’s not recommended to depend on non-GA packages.
- Avoid depending on pre-release, post-release and development release version.
- Dependency version should be final release. (`PEP 440`_)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know what this means.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is actually saying the same thing as the point above, any version which is not a pre-release, post-release and development release is a final release. Removed this point.

Comment thread best_practices/README.rst
- It’s not recommended to depend on non-GA packages.
- Avoid depending on pre-release, post-release and development release version.
- Dependency version should be final release. (`PEP 440`_)
- GA packages must not depend on non-GA packages.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does your use of SHOULD and MUST match RFC terminology: https://www.ietf.org/rfc/rfc2119.txt

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, mentioned this at the top of this file.

Comment thread best_practices/README.rst
- Dependency version should be final release. (`PEP 440`_)
- GA packages must not depend on non-GA packages.

**3. Version range upper bound should be updated when there is a newer version available as soon as possible.**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Didn't you already make this point?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh I was thinking making this a separate point to emphasize on it.. and forgot to delete it from the sub-point in. Removed.

Comment thread best_practices/README.rst

- Packages should use Python built-in modules if possible. e.g. logging, unittest

**5. Never vendor dependencies**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should explain what this means

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

Comment thread best_practices/README.rst Outdated

- Package must be self compatible
If package A depends on dependency B and C, and they require different version
of dependency D, package A is not self compatible. Packages that are not self
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe "internally" rather than self?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

Co-Authored-By: liyanhui1228 <yanhuil@google.com>
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

1 similar comment
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

liyanhui1228 and others added 8 commits October 24, 2018 11:21
@googlebot
Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot
Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

Co-Authored-By: liyanhui1228 <yanhuil@google.com>
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

brianquinlan and others added 8 commits October 29, 2018 16:24
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
@googlebot
Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@liyanhui1228
Copy link
Copy Markdown
Member Author

Ping on this.

Comment thread best_practices/README.rst
- A package makes breaking API changes and doesn't follow `Semantic Versioning`_
- A package has a pinned dependency version which conflicts with other dependencies.
- A package depends on outdated dependencies.
- A package is dependent on deprecated dependencies.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there any way to automate detection of deprecated dependencies? It looks like pypi doesn't really have a concept of a deprecated package.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Currently the deprecated dependency finder tool in our compatibility_lib uses the development status field in setup.py to determine whether a package is deprecated or not. If one pkg is deprecated, the status should be Development Status :: 7 - Inactive.

Comment thread best_practices/README.rst Outdated
Co-Authored-By: liyanhui1228 <yanhuil@google.com>
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot
Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@liyanhui1228 liyanhui1228 merged commit 685a873 into GoogleCloudPlatform:master Nov 15, 2018
@liyanhui1228 liyanhui1228 deleted the guidelines branch November 15, 2018 00:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants