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

Add py310 support #74

Merged
merged 5 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions .github/workflows/deploy-docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ on:
types:
- published

defaults:
run:
shell: bash -l {0}

jobs:
build-docs:
runs-on: ubuntu-latest
Expand All @@ -25,18 +29,16 @@ jobs:
environment-file: false

- name: Build environment
shell: bash -l {0}
run: |
micromamba create --name TEST python=3.9 --file requirements.txt --file requirements-dev.txt --channel conda-forge
micromamba create --name TEST python=3 pip --file requirements-dev.txt --channel conda-forge
micromamba activate TEST
python -m pip install -e . --no-deps --force-reinstall
python -m pip install -e .

- name: Get the version
id: get_version
run: echo ::set-output name=VERSION::$(python setup.py --version)

- name: Build documentation
shell: bash -l {0}
run: |
set -e
micromamba activate TEST
Expand Down
56 changes: 31 additions & 25 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
@@ -1,43 +1,49 @@
name: tests
name: Tests

on:
push: null
pull_request: null
pull_request:
push:
branches: [master]

defaults:
run:
shell: bash -l {0}

jobs:
tests:
name: tests
runs-on: "ubuntu-latest"
run:
runs-on: ${{ matrix.os }}
strategy:
matrix:
python-version: ["3.6", "3.7", "3.8", "3.9", "3.10"]
os: [windows-latest, ubuntu-latest, macos-latest]
fail-fast: false

steps:
- name: cancel previous runs
uses: styfle/cancel-workflow-action@0.6.0
uses: styfle/cancel-workflow-action@0.10.0
with:
access_token: ${{ github.token }}

- uses: actions/checkout@v2
- uses: actions/checkout@v3

- uses: conda-incubator/setup-miniconda@v2
- name: Setup Micromamba
uses: mamba-org/provision-with-micromamba@main
ocefpaf marked this conversation as resolved.
Show resolved Hide resolved
with:
Copy link
Contributor

@zmoon zmoon Aug 3, 2022

Choose a reason for hiding this comment

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

I was thinking about the caching options but I guess that wouldn't help since no conda env yml file. But maybe if use extra-specs as mentioned below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. I only use cache for big envs, this one is not that heavy but we can try that in another PR I already added too much in this one.

python-version: 3.8
channels: conda-forge,defaults
channel-priority: strict
show-channel-urls: true

- name: configure conda and install code
shell: bash -l {0}
environment-file: false
ocefpaf marked this conversation as resolved.
Show resolved Hide resolved

- name: Python ${{ matrix.python-version }}
run: |
conda config --set always_yes yes
conda config --add channels conda-forge
conda install --quiet pip

export GIT_FULL_HASH=`git rev-parse HEAD`
export CICLE_BUILD_URL="https://www.youtube.com/watch?v=R7qT-C-0ajI"
conda install --file requirements.txt --file requirements-dev.txt
micromamba create --name TEST python=${{ matrix.python-version }} pip --file requirements-dev.txt --channel conda-forge
ocefpaf marked this conversation as resolved.
Show resolved Hide resolved
micromamba activate TEST
python -m pip install -e .

- name: test
shell: bash -l {0}
run: |
micromamba activate TEST
coverage run -m pytest -vrsx test.py

- name: coverage
run: |
micromamba activate TEST
coverage report -m
codecov

8 changes: 2 additions & 6 deletions depfinder/inspection.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
from collections import defaultdict
from typing import Union

from stdlib_list import stdlib_list
from .stdliblist import builtin_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just be me but I find stdliblist a bit ugly / hard to read, maybe the module could be stdlib_list_ instead or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Super ugly! I'm terrible with names. I'm OK with stdlib_list_. Let's see what @ericdill thinks and we'll rename.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, let's hold this one for a bit. I forgot I'm also a maintainer of stdlib-list and these changes should be there instead.

Copy link
Owner

Choose a reason for hiding this comment

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

Let's see what @ericdill thinks and we'll rename.

FWIW you don't need my approval for anything in this project. do whatever you think is best. i trust you on this stuff

Copy link
Owner

Choose a reason for hiding this comment

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

i'd prefix this to be _stdlib_list but it's an internal detail so we can fix it in post


from .utils import (
AST_QUESTIONABLE,
Expand All @@ -46,10 +46,6 @@

logger = logging.getLogger('depfinder')

pyver = '%s.%s' % (sys.version_info.major, sys.version_info.minor)
builtin_modules = stdlib_list(pyver)
del pyver


PACKAGE_NAME = None
STRICT_CHECKING = False
Expand All @@ -59,7 +55,7 @@ def get_top_level_import_name(name, custom_namespaces=None):
num_dot = name.count(".")
custom_namespaces = custom_namespaces or []

if name in namespace_packages or name in custom_namespaces or name in stdlib_list():
if name in namespace_packages or name in custom_namespaces or name in builtin_modules:
return name
elif any(
((num_dot - nsp.count(".")) == 1) and name.startswith(nsp + ".")
Expand Down
6 changes: 1 addition & 5 deletions depfinder/reports.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,12 @@
from functools import lru_cache

import requests
from stdlib_list import stdlib_list

from .stdliblist import builtin_modules as _builtin_modules
from .utils import SKETCHY_TYPES_TABLE

logger = logging.getLogger('depfinder')

pyver = '%s.%s' % (sys.version_info.major, sys.version_info.minor)
_builtin_modules = stdlib_list(pyver)
del pyver


@lru_cache()
def _import_map_num_letters():
Expand Down
11 changes: 11 additions & 0 deletions depfinder/stdliblist.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import sys


try:
from stdlib_list import stdlib_list
pyver = '%s.%s' % (sys.version_info.major, sys.version_info.minor)
builtin_modules = stdlib_list(pyver)
del pyver
except ImportError:
# assuming py>=3.10
Copy link
Contributor

@zmoon zmoon Aug 3, 2022

Choose a reason for hiding this comment

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

This should be the case now, but if stdlib-list was somehow missing in Python <3.10 situation, then the error message could be a bit confusing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is listed as a dependency for py<3.10, so the only way that scenario would happen is in a broken installation with "no-deps" and people doing that are usually experts. The error message would be:

AttributeError: module 'sys' has no attribute 'stdlib_module_names'

That is, IMO, the correct information an advanced users should know but I accept suggestions for a better one.

Copy link
Contributor

@zmoon zmoon Aug 3, 2022

Choose a reason for hiding this comment

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

Yeah, I suppose you are right, that makes sense.

I guess I would lean towards switching the try and except parts or doing a sys.version_info check.

And I know you're going another way here, but I also feel like it could be useful to have this in stdlib-list, e.g. to support listing the 3.10 stdlib from an older Python version, and to reduce the number of times this sort of code here would have to be written perhaps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. It makes sense to put in stliblib-list. I completely forgot I am a maintainer there and I can port this over tomorrow.

builtin_modules = list(set(list(sys.stdlib_module_names) + list(sys.builtin_module_names)))
8 changes: 1 addition & 7 deletions depfinder/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,7 @@

import requests
import yaml
from stdlib_list import stdlib_list

logger = logging.getLogger('depfinder')

pyver = '%s.%s' % (sys.version_info.major, sys.version_info.minor)
builtin_modules = stdlib_list(pyver)
del pyver
from .stdliblist import builtin_modules


SKETCHY_TYPES_TABLE = {}
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
pyyaml
stdlib-list
stdlib-list; python_version < "3.10"
requests
40 changes: 29 additions & 11 deletions test.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,6 @@ def __init__(self):
# Hit the fake packages code block in main.sanitize_deps()
{'targets': {'required': ['numpy']},
'code': 'from numpy import warnings as npwarn'},
# Test for nested namespace resolution in stdlib builtins
{'targets': {'builtin': ['concurrent.futures']},
'code': 'import concurrent.futures'},
]

relative_imports = [
Expand All @@ -119,6 +116,27 @@ def __init__(self):
'code': 'from ..bar import baz'},
]

@pytest.fixture(scope='module')
def using_stdlib_list():
try:
import stdlib_list
return True
except ImportError:
return False

def test_nested_namespace_builtins(using_stdlib_list):
if using_stdlib_list:
expected = {'builtin': ['concurrent.futures']}
else:
expected = {'builtin': ['concurrent']}
code = 'import concurrent.futures'



test_object = Initter({'targets': expected, 'code': code})
imports = main.get_imported_libs(test_object.code)
assert imports.describe() == test_object.targets


class Initter(object):
def __init__(self, artifact):
Expand Down Expand Up @@ -336,8 +354,7 @@ def test_cli(path, req, capsys):
if req is None:
dependencies_file = join(dirname(dirname(depfinder.__file__)),
'requirements.txt')
dependencies = set(
[dep for dep in open(dependencies_file, 'r').read().split('\n') if dep])
dependencies = set([dep for dep in open(dependencies_file, 'r').read().split('\n') if not dep.startswith("stdlib")])
else:
dependencies = req
assert dependencies == set(eval(stdout).get('required', set()))
Expand Down Expand Up @@ -390,9 +407,9 @@ def test_get_top_level_import():


def test_report_conda_forge_names_from_import_map():
m, f, c = parse_file(join(dirname(depfinder.__file__), 'inspection.py'))
m, f, c = parse_file(join(dirname(depfinder.__file__), 'utils.py'))
report, import_to_artifact, import_to_pkg = report_conda_forge_names_from_import_map(c.total_imports)
assert report['required'] == {'stdlib-list'}
assert report['required'] == {'pyyaml', 'requests'}


def test_report_conda_forge_names_from_import_map_ignore():
Expand All @@ -404,8 +421,9 @@ def test_report_conda_forge_names_from_import_map_ignore():

def test_simple_import_search_conda_forge_import_map():
path_to_source = dirname(depfinder.__file__)
expected_result = sorted(list({"pyyaml", "requests"}))
report = simple_import_search_conda_forge_import_map(path_to_source)
assert report['required'] == sorted(list({"pyyaml", "stdlib-list", "requests"}))
assert report['required'] == expected_result


@pytest.mark.parametrize('import_name, expected_result', [
Expand All @@ -431,8 +449,8 @@ def test_search_for_name(import_name, expected_result):
def test_simple_import_to_pkg_map():
path_to_source = dirname(depfinder.__file__)
import_to_artifact = simple_import_to_pkg_map(path_to_source)
assert import_to_artifact == {'builtin': {},
'questionable': {'IPython.core.inputsplitter': {'ipython', 'autovizwidget'}},
expected_result = {'builtin': {},
'questionable': {'stdlib_list': {'stdlib-list'}, 'IPython.core.inputsplitter': {'ipython', 'autovizwidget'}},
'questionable no match': {},
'required': {'requests': {'apache-libcloud',
'arm_pyart',
Expand All @@ -441,6 +459,6 @@ def test_simple_import_to_pkg_map():
'google-api-core',
'google-cloud-bigquery-storage-core',
'requests'},
'stdlib_list': {'stdlib-list'},
'yaml': {'google-cloud-bigquery-storage-core', 'pyyaml'}},
'required no match': {}}
assert import_to_artifact == expected_result