-
Notifications
You must be signed in to change notification settings - Fork 80
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
Remove duplicate dictionary keys #28
Conversation
@andrewda Excellent work! There are a few inputs I tried that caused it to crash though: lambda x: {(0,1): 1, (0,1): 2} {(0,1): 1, (0,1): 2}
I got the examples from the https://github.com/pycqa/pyflakes/blob/master/pyflakes/test/test_dict.py Thanks |
3aa1d91
to
3275242
Compare
@myint Thanks for checking that! I had no idea object keys could be tuples in Python. Should be fixed now! |
Would be good to add a test case for more complex data types being used in the dict keys. |
coverage is still decreasing. That means there are new code paths which are not being tested. |
That could be my fault. Try setting the environment variable |
Actually, I think you'll need to add it to the https://github.com/hhatto/autopep8/blob/4c6a018b7d17b8414155db51abe21adc7e38d496/Makefile#L47 |
@jayvdb Any idea why
aren't covered? Adding prints there shows them to be running fine locally. Edit: nevermind, what @myint said might fix those |
I'm noticing that I did things a bit differently here than in https://github.com/myint/autoflake/blob/b4824635ecdec24a87c32ad6a1c61f6dce09b785/.travis.yml#L26 |
my guess is https://coveralls.io/builds/14875802/source?filename=autoflake.py#L149 is getting covered, but only by some of the jobs. I dont know coveralls.io very well; I use codecov.io exclusively as it merges reports from multiple CI builds, allowing 100% coverage to be reached even for Windows and Unix specific code. |
And if that doesn't work, you could always just test one of the higher-level functions like in: That way, you don't need to depend on my test coverage subprocess code working. It wouldn't surprise me if there is something wrong with it given that that code was never being exercised previously. |
@myint E2E tests failing with |
I think
So instead of in
|
917ff27
to
51f52c9
Compare
I just noticed something else missing in https://github.com/myint/autoflake/blob/b4824635ecdec24a87c32ad6a1c61f6dce09b785/.travis.yml#L26 you need:
|
|
@jayvdb I tested it locally. It does work using https://github.com/myint/autoflake/blob/master/test_autoflake.py#L24-L37 |
By the way, the massive decrease is expected given that the results aren't being combined. Hence my previous comment. 😄 |
You might also need diff --git a/.travis.yml b/.travis.yml
index 8382206..3217848 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -23,7 +23,7 @@ script:
after_success:
- pip install --quiet coverage
- - AUTOFLAKE_COVERAGE=1 coverage run --include='autoflake.py,test_autoflake.py' test_autoflake.py
+ - AUTOFLAKE_COVERAGE=1 coverage run --branch --parallel-mode --include='autoflake.py,test_autoflake.py' test_autoflake.py
- coverage combine
- coverage report --show-missing |
Oh I see |
Here is the output when I test this locally with diff --git a/Makefile b/Makefile
index a469920..5bb3065 100644
--- a/Makefile
+++ b/Makefile
@@ -12,7 +12,8 @@ check:
coverage:
@coverage erase
- @AUTOFLAKE_COVERAGE=1 ${coverage} run test_autoflake.py
+ @AUTOFLAKE_COVERAGE=1 coverage run --branch --parallel --include='autoflake.py,test_autoflake.py' test_autoflake.py
+ @coverage combine
@coverage report
@coverage html
@python -m webbrowser -n "file://${PWD}/htmlcov/index.html"
|
This will automatically remove duplicate dictionary keys with the `--remove-duplicate-keys` argument. Closes PyCQA#27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! And thanks for discovering/fixing my coverage bug!
This will automatically remove duplicate dictionary keys with the
--remove-duplicate-keys
argument.Closes #27
Example