Initial API package layout #19
Conversation
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from .version import __version__ |
There was a problem hiding this comment.
In OpenCensus we avoid the relative import. I wonder how people think about using relative versus absolute import in OpenTelemetry.
The following article explored the pros and cons.
https://realpython.com/absolute-vs-relative-python-imports/
There was a problem hiding this comment.
I like relative imports and use them in personal projects. The google style guide says to import modules using the full path, but the justification for doing this only applies to implicit relative imports:
Avoids conflicts in module names or incorrect imports due to the module search path not being what the author expected
Since implicit relative imports are gone in python 3 I think this is fine. If we decide to use it we should make sure we use this import style everywhere.
There was a problem hiding this comment.
@carlosalberto @Oberon00 I'll merge the PR first, please comment here if you see potential issue that could bite us. Thanks.
|
I signed it |
| include_package_data=True, | ||
| long_description=open('README.rst').read(), | ||
| install_requires=[ | ||
| ], |
There was a problem hiding this comment.
I put this way as we will add the contextvars backport dependency, which will be one line change instead of three.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| __version__ = "0.1.0" |
There was a problem hiding this comment.
Why is this in internal/ instead of the top level opentelemetry/?
There was a problem hiding this comment.
Put it in internal/ since version is not part the specification yet.
We don't have a good way to put it under top level opentelemetry/ if we want multiple packages to share the same namespace (namespace package).
There was a problem hiding this comment.
Also, as we make a namespace package, one could, in theory, have different versions of SDK and API subpackages, so it doesn't even make sense semantically to put the version out at the top level.
| @@ -0,0 +1 @@ | |||
| __path__ = __import__("pkgutil").extend_path(__path__, __name__) | |||
There was a problem hiding this comment.
Since we don't support Python 2 anyway (and probably not <3.3 too), why use the pkgutil style and not the implicit, native style for namespace packages? https://packaging.python.org/guides/packaging-namespace-packages/#native-namespace-packages
Caveat: I've never created a namespace package before, so this is just me reading the docs and wondering.
There was a problem hiding this comment.
Good point, I'll update it in my next PR.
|
|
||
| from setuptools import find_packages | ||
| from setuptools import setup | ||
| from opentelemetry.internal import __version__ |
There was a problem hiding this comment.
I thought that does not work? I mean, importing from the package you are about to install but that is not yet installed. See https://packaging.python.org/guides/single-sourcing-package-version/. That guide notes that such an import can fail if some packages from install_requires would be required. I think, since we already have a dedicated version file now, we could use approach 3: "Set the value to a version global variable in a dedicated module in your project (e.g. version.py), then have setup.py read and exec the value into a variable."
There was a problem hiding this comment.
I'll investigate. Approach 3 looks reasonable.
| license="Apache-2.0", | ||
| packages=find_packages(exclude=("examples", "tests",)), | ||
| namespace_packages=[], | ||
| url="https://github.com/open-telemetry/opentelemetry-python/opentelemetry-api", |
There was a problem hiding this comment.
That's a 404. You'd need to add /blob/master in the URL.
No description provided.