refactor(util.uri): refactor dict comprehension #1783

withshubh · 2020-11-16T08:00:51Z

This PR fixes some of the quality issues raised by DeepSource on my fork of this repository.

Some of them are:

Performance Issues: 4
Bug Risks: 14
Anti-Patterns: 24
Security: 6

Take a quick look at all the issues caught by DeepSource for this repository here

Summary of Changes

~~Change methods not using its bound instance to staticmethods~~
~~use literal syntax instead of function calls to create data structure~~
~~remove assert statement from non-test files~~
~~refactor unnecessary 'else' / 'elif' when 'if' block has a 'return' statement~~
~~replace ternary syntax with if expression~~
~~remove methods with unnecessary super delegation~~
remove unnecessary generator

You can also have a look at the configuration file I used for DeepSource Analysis.

If any of these issues that the DeepSource analysis found are critical for the codebase I'd suggest you to activate the analysis. You can trigger analysis on each PR once you integrate DeepSource in the repo. ⚡

Feel free to merge this PR if you wish to fix the issues.✨

vytas7 · 2020-11-16T08:05:13Z

Hi, and thanks for this!

I'm, however, unsure if we want to refactor everything to @staticmethod. In some places, methods semantically belong to a class instance, even if they do not use self. In others, it probably does not warrant a change.

@kgriffs @CaselIT thoughts?

CaselIT · 2020-11-16T08:53:37Z

I'm definitely -1 on the static method thing.

vytas7 · 2020-11-16T20:54:17Z

Another comment unrelated to the use of @staticmethod.

Sorry to be blunt, but refactoring assert statements to manually raising AssertionError makes no sense to me.
The assert statement is the canonical way of writing tests with pytest. Manually raising an AssertionError in a conditional branch may even interfere with pytest's ability to infer useful failure messages.

vytas7 · 2020-11-16T20:58:54Z

falcon/util/uri.py

@@ -49,9 +49,9 @@
 _HEX_DIGITS = '0123456789ABCDEFabcdef'

 # This map construction is based on urllib's implementation
-_HEX_TO_BYTE = dict(((a + b).encode(), bytes([int(a + b, 16)]))
+_HEX_TO_BYTE = {(a + b).encode(): bytes([int(a + b, 16)])


I guess this change we could actually keep. The old form probably could not use dict comprehension because it had to support Python 2.6.

vytas7 · 2020-11-16T21:01:41Z

tests/test_sinks.py

@@ -23,8 +24,7 @@ def __call__(self, req, resp, **kwargs):


 class SinkAsync(Sink):
-    async def __call__(self, req, resp, **kwargs):


Reimplementing __call__ is intended here (note the change from sync to async).

Tbh, this looks like a bug in DeepSource if the above is marked for refactoring.

vytas7

Thank you once again for this initiative to improve our code quality.

However, as discussed in the above comments, we do not want to indiscriminately refactor the following:

Functions not making use of self ➡️ @staticmethod. @staticmethod may at times semantically make sense, but more often than not, it is not the case in our codebase. Moreover, it does not offer any speed improvement (it actually may even incur a slight penalty).
assert ➡️ manually raising an AssertionError in a branch statement -- we simply do not want this.
elif after return or raise ➡️ if. While I have nothing against the pattern itself, and actually use it, we need to discuss whether it is worth the effort (e.g., how do these alternatives compare performance wise?) refactoring. I.e., I'm actually not opposed to this one, but it is better to discuss this one separately.

I know it feels like harsh feedback... 👿 But could you roll back refactoring of the 3 above patterns, and we could take a look at what's left. As noted on the PR, there are some good changes that we could actually be willing to keep.

Notwithstanding potential improvements, you also have to get our CI to pass in order for a PR to be considered. Check how to run tox or our mintest.sh wrapper here: CONTRIBUTING.md. Right now, it seems that some tests got broken because of refactoring, including the simplest of smoke tests.

Needless to say, @withshubh, You are also very welcome to discuss these changes with us on our Gitter channels!

codecov · 2020-11-18T07:32:15Z

Codecov Report

Merging #1783 (e6d4681) into master (c7ab8da) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1783   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           54        54           
  Lines         5128      5128           
  Branches       823       823           
=========================================
  Hits          5128      5128

Impacted Files	Coverage Δ
falcon/util/uri.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ab8da...e6d4681. Read the comment docs.

vytas7 · 2020-12-10T07:24:11Z

@withshubh just checking, if you are planning to continue work on this PR?

And I'm sorry to be blunt (again), but an en masse autopep8 refactoring is not something we want to accept as a side effect of this PR. We are looking into adopting black, autopep8 or other alternatives, but that needs to be a separate, informed decision by the maintainer team. You'll have to revert those changes from your branch in order to proceed.

withshubh · 2020-12-10T08:12:45Z

Hi @vytas7 👋

It's okay for being blunt(again) 😛 I'll fix this and update the PR. ✨

Revert fixes breaking checks and fixes made using autopep8 transformer

withshubh · 2020-12-10T11:16:15Z

Hi @vytas7

Please have a look. 👀

vytas7

Please see inline comments.

vytas7 · 2020-12-10T13:38:00Z

.deepsource.toml

@@ -0,0 +1,10 @@
+version = 1


Please remove this file.
We have not decided on using DeepSource yet.

vytas7 · 2020-12-10T13:38:55Z

falcon/util/structures.py

@@ -61,7 +61,7 @@ class CaseInsensitiveDict(MutableMapping):  # pragma: no cover

    """
    def __init__(self, data=None, **kwargs):
-        self._store = dict()
+        self._store = {}


Please read the comment on line 33 to understand why, and revert this change.

vytas7 · 2020-12-10T13:41:10Z

falcon/vendor/mimeparse/mimeparse.py

@@ -98,7 +98,7 @@ def quality_and_fitness_parsed(mime_type, parsed_ranges):
        if type_match and subtype_match:

            # 100 points if the type matches w/o a wildcard
-            fitness = type == target_type and 100 or 0
+            fitness = 100 if type == target_type else 0


Let us revert these changes. Sensible they may be, we have vendored this module from https://github.com/dbtsai/python-mimeparse, and do not want to make any modifications unless absolutely necessary.

vytas7 · 2020-12-10T13:41:31Z

falcon/vendor/mimeparse/mimeparse.py

@@ -181,7 +181,7 @@ def best_match(supported, header):
        pos += 1
    weighted_matches.sort()

-    return weighted_matches[-1][0][0] and weighted_matches[-1][2] or ''
+    return weighted_matches[-1][2] if weighted_matches[-1][0][0] else ''


Let us revert these changes. Sensible they may be, we have vendored this module from https://github.com/dbtsai/python-mimeparse, and do not want to make any modifications unless absolutely necessary.

…ata structure, Replace ternary syntax with if expression

revert fix

withshubh · 2020-12-11T07:41:07Z

Hi @vytas7

Now this PR contains just one fix:

remove unnecessary generator

I'd like to know what are your thoughts regarding integrating the DeepSource.

vytas7

Let us please address PEP8 violations before merging this PR:

./falcon/util/uri.py:53:21: E127 continuation line over-indented for visual indent
./falcon/util/uri.py:54:21: E127 continuation line over-indented for visual indent

withshubh · 2020-12-19T08:04:39Z

Let us please address PEP8 violations before merging this PR:

./falcon/util/uri.py:53:21: E127 continuation line over-indented for visual indent
./falcon/util/uri.py:54:21: E127 continuation line over-indented for visual indent

Done @vytas7 ✨

vytas7

Thanks for this!

Regarding DeepSource, my impression is that it is likely a very useful tool for a business project, or even a new open source library if DeepSource is agreed upon in advance.
However, for a highly specialized framework Falcon is, as you may have noticed, many "quality issues" exist there for a reason ("vendored" code, explicit test scenarios, deprecation strategies etc).
Moreover, another drawback is that DeepSource is not open, so we cannot easily integrate it into our offline development routines.

vytas7 reviewed Nov 16, 2020

View reviewed changes

vytas7 requested changes Nov 16, 2020

View reviewed changes

deepsourcebot and others added 8 commits December 10, 2020 14:25

Initial fixes and formatting using autopep8

5918707

revert fixes and updated master with upstream branch

f8857b7

Add .deepsource.toml

e1e67de

Use literal syntax instead of function calls to create data structure

4e8eb7a

Remove unnecessary generator

5a7b335

Replace ternary syntax with if expression

02ca75d

Merge pull request #36 from withshubh/deepsource1

a30357d

Revert fixes breaking checks and fixes made using autopep8 transformer

Merge branch 'master' into master

5c27267

vytas7 requested changes Dec 10, 2020

View reviewed changes

withshubh added 4 commits December 11, 2020 12:55

revert fix - Use literal syntax instead of function calls to create d…

738e8bb

…ata structure, Replace ternary syntax with if expression

Merge pull request #37 from withshubh/deepsource1

ee7a7a6

revert fix

Delete .deepsource.toml

5433166

Update .gitignore

6589626

vytas7 requested changes Dec 16, 2020

View reviewed changes

vytas7 changed the title ~~Potential bug-fixes using DeepSource~~ refactor(util.uri): refactor dict comprehension Dec 16, 2020

fixed pep8 voilation occuring in ci check

944042e

Merge branch 'master' into master

e6d4681

vytas7 approved these changes Dec 19, 2020

View reviewed changes

vytas7 merged commit ba78a52 into falconry:master Dec 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(util.uri): refactor dict comprehension #1783

refactor(util.uri): refactor dict comprehension #1783

withshubh commented Nov 16, 2020 •

edited

Loading

vytas7 commented Nov 16, 2020

CaselIT commented Nov 16, 2020

vytas7 commented Nov 16, 2020

vytas7 Nov 16, 2020

vytas7 Nov 16, 2020 •

edited

Loading

vytas7 left a comment •

edited

Loading

codecov bot commented Nov 18, 2020 •

edited

Loading

vytas7 commented Dec 10, 2020

withshubh commented Dec 10, 2020

withshubh commented Dec 10, 2020

vytas7 left a comment

vytas7 Dec 10, 2020

vytas7 Dec 10, 2020

vytas7 Dec 10, 2020

vytas7 Dec 10, 2020

withshubh commented Dec 11, 2020

vytas7 left a comment

withshubh commented Dec 19, 2020

vytas7 left a comment •

edited

Loading

		@@ -23,8 +24,7 @@ def __call__(self, req, resp, **kwargs):


		class SinkAsync(Sink):
		async def __call__(self, req, resp, **kwargs):

refactor(util.uri): refactor dict comprehension #1783

refactor(util.uri): refactor dict comprehension #1783

Conversation

withshubh commented Nov 16, 2020 • edited Loading

Summary of Changes

vytas7 commented Nov 16, 2020

CaselIT commented Nov 16, 2020

vytas7 commented Nov 16, 2020

vytas7 Nov 16, 2020

Choose a reason for hiding this comment

vytas7 Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

vytas7 left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2020 • edited Loading

Codecov Report

vytas7 commented Dec 10, 2020

withshubh commented Dec 10, 2020

withshubh commented Dec 10, 2020

vytas7 left a comment

Choose a reason for hiding this comment

vytas7 Dec 10, 2020

Choose a reason for hiding this comment

vytas7 Dec 10, 2020

Choose a reason for hiding this comment

vytas7 Dec 10, 2020

Choose a reason for hiding this comment

vytas7 Dec 10, 2020

Choose a reason for hiding this comment

withshubh commented Dec 11, 2020

vytas7 left a comment

Choose a reason for hiding this comment

withshubh commented Dec 19, 2020

vytas7 left a comment • edited Loading

Choose a reason for hiding this comment

withshubh commented Nov 16, 2020 •

edited

Loading

vytas7 Nov 16, 2020 •

edited

Loading

vytas7 left a comment •

edited

Loading

codecov bot commented Nov 18, 2020 •

edited

Loading

vytas7 left a comment •

edited

Loading