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

Fix inotify behavior when presented with non-ASCII path events #519

Merged
merged 9 commits into from
Mar 9, 2019

Conversation

exarkun
Copy link
Contributor

@exarkun exarkun commented Feb 1, 2019

Fixes #516

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 13, 2019

Hello @exarkun , thanks for the patch!

Could you change that line to:

python -bb -m pytest {posargs}

It will ensure there is no BytesWarning.

@exarkun
Copy link
Contributor Author

exarkun commented Feb 13, 2019

Hello @exarkun , thanks for the patch!

Could you change that line to:

python -bb -m pytest {posargs}

It will ensure there is no BytesWarning.

Sure thing. Done.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Feb 15, 2019

Thanks for the changes, and I am sorry to ask you one last thing :)
I was trying to reproduce the error that was fixed with the commit you are partially reverting with this PR. This is simply to ensure no regression.

Could you apply those changes (I should have done it way sooner):

diff --git a/tests/conftest.py b/tests/conftest.py
index ed58af3..418a58d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -20,3 +20,22 @@ def p(tmpdir, *args):
     with the provided arguments.
     """
     return partial(os.path.join, tmpdir)
+
+
+
+@pytest.fixture(autouse=True)
+def no_warnings(recwarn):
+    """Fail on warning."""
+
+    yield
+
+    warnings = []
+    for warning in recwarn:  # pragma: no cover
+        message = str(warning.message)
+        if (
+            "Not importing directory" in message
+            or "Using or importing the ABCs" in message
+        ):
+            continue
+        warnings.append("{w.filename}:{w.lineno} {w.message}".format(w=warning))
+    assert not warnings

With this fixture, you will see several failures related to your changes and this is what we tried to fix in the older commit.

Many many thanks for your patience, time and work 💪

@exarkun
Copy link
Contributor Author

exarkun commented Mar 8, 2019

@BoboTiG The no-warnings fixture is now in place and I've fixed the test failures where such warnings were being triggered (probably; macOS travis jobs still running, the rest are green).

@BoboTiG BoboTiG merged commit 9852970 into gorakhargosh:master Mar 9, 2019
@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 9, 2019

Thanks @exarkun :)

@exarkun exarkun deleted the 516.inotify-vs-unicode branch March 12, 2019 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants