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

Import Hashable from collections.abc #181

Closed
wants to merge 2 commits into from
Closed

Import Hashable from collections.abc #181

wants to merge 2 commits into from

Conversation

@The-Compiler
Copy link
Contributor

@The-Compiler The-Compiler commented Jun 27, 2018

In Python 3.7, importing ABCs directly from the collections module shows a
warning (and in Python 3.8 it will stop working) - see
python/cpython@c66f9f8

Since this is only done in lib3/ which is Python 3 only, we can unconditionally
import it from collections.abc instead.

This fixes the following DeprecationWarning:

.../site-packages/yaml/__init__.py:75: in load
    return loader.get_single_data()
.../site-packages/yaml/constructor.py:37: in get_single_data
    return self.construct_document(node)
.../site-packages/yaml/constructor.py:46: in construct_document
    for dummy in generator:
.../site-packages/yaml/constructor.py:398: in construct_yaml_map
    value = self.construct_mapping(node)
.../site-packages/yaml/constructor.py:204: in construct_mapping
    return super().construct_mapping(node, deep=deep)
.../site-packages/yaml/constructor.py:126: in construct_mapping
    if not isinstance(key, collections.Hashable):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

name = 'Hashable'

    def __getattr__(name):
        # For backwards compatibility, continue to make the collections ABCs
        # through Python 3.6 available through the collections module.
        # Note, no new collections ABCs were added in Python 3.7
        if name in _collections_abc.__all__:
            obj = getattr(_collections_abc, name)
            import warnings
            warnings.warn("Using or importing the ABCs from 'collections' instead "
                          "of from 'collections.abc' is deprecated, "
                          "and in 3.8 it will stop working",
>                         DeprecationWarning, stacklevel=2)
E           DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
In Python 3.7, importing ABCs directly from the 'collections' module shows a
warning (and in Python 3.8 it will stop working) - see
python/cpython@c66f9f8

Since this is only done in lib3/ which is Python 3 only, we can unconditionally
import it from collections.abc instead.

This fixes the following DeprecationWarning:

.../site-packages/yaml/__init__.py:75: in load
    return loader.get_single_data()
.../site-packages/yaml/constructor.py:37: in get_single_data
    return self.construct_document(node)
.../site-packages/yaml/constructor.py:46: in construct_document
    for dummy in generator:
.../site-packages/yaml/constructor.py:398: in construct_yaml_map
    value = self.construct_mapping(node)
.../site-packages/yaml/constructor.py:204: in construct_mapping
    return super().construct_mapping(node, deep=deep)
.../site-packages/yaml/constructor.py:126: in construct_mapping
    if not isinstance(key, collections.Hashable):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

name = 'Hashable'

    def __getattr__(name):
        # For backwards compatibility, continue to make the collections ABCs
        # through Python 3.6 available through the collections module.
        # Note, no new collections ABCs were added in Python 3.7
        if name in _collections_abc.__all__:
            obj = getattr(_collections_abc, name)
            import warnings
            warnings.warn("Using or importing the ABCs from 'collections' instead "
                          "of from 'collections.abc' is deprecated, "
                          "and in 3.8 it will stop working",
>                         DeprecationWarning, stacklevel=2)
E           DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
alex
alex approved these changes Jun 27, 2018
@webknjaz
Copy link

@webknjaz webknjaz commented Nov 4, 2018

This is included in #172

Loading

@edmorley
Copy link

@edmorley edmorley commented Feb 11, 2019

Please can this be merged/released? :-)

Loading

@ingydotnet
Copy link
Member

@ingydotnet ingydotnet commented Feb 28, 2019

This request has been merged into the release/5.1 branch and is expected to be in the 5.1 release, coming soon. It will be in the 5.1b3 which is being prepared now.

Loading

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 14, 2019

merged in 9959328

Loading

@perlpunk perlpunk closed this Mar 14, 2019
@webknjaz
Copy link

@webknjaz webknjaz commented Mar 14, 2019

@perlpunk why don't you use normal merges visible to everyone?

Loading

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Mar 14, 2019

@webknjaz Several reasons.

These PRs were applied by us to the release branch instead of master, and we (@ingydotnet and me) are usually not using the web, but do most things from commandline.
We are aware that PRs don't show as merged this way (only if they target the current HEAD).

Our preferences on how to merge (rebase, squash, marge commits) are different, and we try to make this better in the future.

Loading

@webknjaz
Copy link

@webknjaz webknjaz commented Mar 14, 2019

@perlpunk yeah, but no one sees this then. Visibility matters. I mostly use CLI too, it's great. I'm just advocating for better interlinking.

Also, if you use git merge command on the base branch (master in this case) and push that merge commit GitHub will figure out that it's merged.

There's a few possible improvements I can suggest:

  1. Hit Edit next to PR title and change the base branch to release. This will also ensure to run public CI properly etc etc... And when pushing manually merged commit will close the PR as merged as long as all commits in PR branch are included there.
  2. Amend commit to have Closes #181 and GitHub will also link this properly giving more visibility on the closure reason.
  3. Regarding rebase/squash/merge, I think you already know downsides... I personally prefer merges as they preserve proper tree history but if you have to use non-merge ways, please include keywords with references GitHub recognizes for better interlinking.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5.1 Release
Added to release/5/1 branch
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants