feat(typing): type request module #2271

CaselIT · 2024-08-14T21:51:44Z

Type sync and async request modules.

Also move the attribute docs next to the actual attribute declaration, to improve usability with editors.

Some additional odd and end typing improvements

Also incorporate #2205:

Co-authored-by: Dave Tapley <dave@tapley.com>

codecov · 2024-08-14T21:55:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (df2debe) to head (9f8231b).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2271   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         7249      7326   +77     
  Branches      1268      1275    +7     
=========================================
+ Hits          7249      7326   +77

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CaselIT

@vytas7 I've marked a couple of point where I've done something slightly different. Honestly not sure we gain much by doing that change

CaselIT · 2024-08-14T22:08:45Z

falcon/asgi/request.py

-        #   gets a None back on the first reference to property, it
-        #   probably isn't going to access the property again (TBD).
-        if self._cached_if_match is None:
+        if self._cached_if_match is MISSING:


this is different

I prefer MISSING than None.

CaselIT · 2024-08-14T22:08:54Z

falcon/asgi/request.py

-    def if_none_match(self):
-        if self._cached_if_none_match is None:
+    def if_none_match(self) -> Optional[List[Union[ETag, Literal['*']]]]:
+        if self._cached_if_none_match is MISSING:


CaselIT · 2024-08-14T22:17:41Z

falcon/request.py

-        #   gets a None back on the first reference to property, it
-        #   probably isn't going to access the property again (TBD).
-        if self._cached_if_match is None:
+        if self._cached_if_match is MISSING:


this is different

CaselIT · 2024-08-14T22:17:49Z

falcon/request.py

+
+        (See also: RFC 7232, Section 3.2)
+        """
+        if self._cached_if_none_match is MISSING:


this is different

CaselIT · 2024-08-15T09:23:37Z

@vytas7 it's ready for review. We need to decide what to do with python 3.7

I vote to drop it. We can do best effort for 3.8 in 4.0, but let's keep the mention that it may be dropped in following 4.x releases

vytas7 · 2024-08-15T09:49:49Z

@vytas7 it's ready for review. We need to decide what to do with python 3.7

I vote to drop it. We can do best effort for 3.8 in 4.0, but let's keep the mention that it may be dropped in following 4.x releases

Yes, all that is already described in the upcoming release notes: Changes to Supported Platforms.

Re 3.7, cannot we just do Protocol = object in typing.py for 3.7, and things would just work? We could just write that typing is unsupported on 3.7. And then drop 3.7 and maybe 3.8 in Falcon 4.1.

CaselIT · 2024-08-15T13:03:04Z

Re 3.7, cannot we just do Protocol = object in typing.py for 3.7, and things would just work? We could just write that typing is unsupported on 3.7. And then drop 3.7 and maybe 3.8 in Falcon 4.1.

I'm not really sold on doing this. Since I don't think type checkers will like it. In my opinion we either do something like https://github.com/falconry/falcon/blob/7341f96eab52f98b16080f2f458fa5fd3799f4f5/falcon/_typing_extensions.py or we drop it.
Doing the aliasing seems an hack

# Conflicts: # falcon/request.py

# Conflicts: # falcon/typing.py # falcon/util/uri.py

falcon/media/multipart.py

CaselIT · 2024-08-24T10:17:21Z

not sure what's up with coverage.

vytas7

Looks great, thanks! 👍

I would just like to make a decision on that MISSING constant before proceeding -- either we need to document it as part of the public interface, or underscore it.

falcon/constants.py

falcon/util/uri.py

CaselIT · 2024-08-25T21:07:04Z

re missing, I've moved it to the typing module, so that we can decide with all the rest when we are done with the typing what to make public and not to

vytas7 · 2024-08-26T05:59:45Z

falcon/constants.py

@@ -184,7 +184,8 @@

 # NOTE(kgriffs): Special singleton to be used internally whenever using
 #   None would be ambiguous.
-_UNSET = object()
+_UNSET = object()  # TODO: remove once replaced with missing


"Unset" sounds better to me than "missing" in the general case. Or maybe, for things request, "missing" sounds good too, but for response properties it would feel odd, like for resp.media it is more logical to say it is either set or unset.

In general a value can be missing. That's where the name comes from. I'm not attached to it by any means, it's just what I use at work (that I think I've copied from some other project)

We can leave a todo in the enum to rename and do that once the _UNSET object has been removed, so we avoid the confusion?

That works for me, but regardless of the name, let's underscore it for any runtime usage (i.e. outside of type annotations), so we can change it at any point as we wish.

vytas7 · 2024-08-26T07:55:51Z

falcon/typing.py

+
+
+_T = TypeVar('_T')
+MISSING = _Missing.MISSING


Could we underscore this constant too then, if we are using it outside of this module?

my idea was to cleanup all the typing aliases at the same time in a final PR once all things are typed

vytas7 · 2024-08-26T07:59:22Z

falcon/request.py

+    _cookies_collapsed: Optional[Dict[str, str]] = None
+    _cached_if_match: MissingOr[Optional[List[Union[ETag, Literal['*']]]]] = MISSING
+    _cached_if_none_match: MissingOr[Optional[List[Union[ETag, Literal['*']]]]] = (
+        MISSING


Could we keep None for now? This PR isn't the smallest to follow, so ideally we should minimize the introduced runtime changes outside of typing. We probably need to profile things again otherwise (although these cached headers are probably not the most/important sensitive part; we should move the ones that are less frequently used to being cached in some _state attribute to increase perf even more).

I don't think we need to profile things for this change, since a comparison using is has the same time.

missing is used when the final value can include none. in those cases having a default of None is suboptimal like mentioned in the previous TODO.

I can create a new PR for this, but seemed so small a change that made sense to do inline with the typing one

I don't think this is true for cythonized code, but since these methods are not very critical for core performance of the framework, I agree it is no big deal.

Just for reference

_OBJECT = object() def func1(obj): return obj is None def func2(obj): return obj is _OBJECT

⬇️

..................... func1 (None is None): Mean +- std dev: 31.5 ns +- 0.4 ns ..................... func1 (_OBJECT is None): Mean +- std dev: 32.0 ns +- 2.0 ns ..................... func2 (None is _OBJECT): Mean +- std dev: 37.0 ns +- 0.6 ns ..................... func2 (_OBJECT is _OBJECT): Mean +- std dev: 36.9 ns +- 1.0 ns

This is because is None in Cython can be implemented as checking a C pointer vs NULL, whereas a global or local constant still needs to be resolved from name first.

But as said, I don't think this difference is enough to bother. But I do want to underscore these constants when used in runtime, so _Missing, _MISSING, _UNSET, etc.

as mentioned my idea was to worry about the public/private in a final cleanup PR. but if you prefer it here I can change it here.

personally I think all the properties could be re-written using a memorizing property since request has dict in any case

OK, but let's not forget about that cleanup then!

roger. Should we also try the momoizing prop approach?

I think it would end up faster both in init than in use

Dunno, we could maybe focus on getting 4.0 out the door first, and then circle back on rebuilding benchmarks, Codspeed, and optimizations.

I was thinking to add a private ._state attribute, object or dict (whatever suits better), and then store less used cached header properties there. Also store stuff like the state of dependent middleware which we would deprecate and provide a shim for.

if we keep __dict__ in request, memoized properties are all faster, since they avoid the property after the first run

Re cleanup/privatizing, I added a note on #1457.

but sure let's just keep it in mind

copalco and others added 5 commits August 14, 2024 22:29

Type request module

26ecfd7

types: improve types in media, request helpers

7b99a85

types: type sync request

0760181

types: type async request

b781e73

typing: additional improvements and clanup

16e1e7a

CaselIT mentioned this pull request Aug 14, 2024

Type request module #2270

Closed

13 tasks

CaselIT requested a review from vytas7 August 14, 2024 21:52

typing: minor overload improvements and removal of unnecessary casts

e4c5eb9

CaselIT commented Aug 14, 2024

View reviewed changes

CaselIT marked this pull request as draft August 14, 2024 22:37

CaselIT added 3 commits August 15, 2024 10:40

style: fix docs linter issues

fe80993

fix: catch warning and fix typing errors

7575d95

fix: allow untyped defs in the test package

18738bc

CaselIT marked this pull request as ready for review August 15, 2024 09:22

CaselIT mentioned this pull request Aug 15, 2024

docs: normalize Sphinx link markers #2272

Merged

vytas7 changed the title ~~Type request module~~ feat(typing): type request module Aug 15, 2024

CaselIT added 2 commits August 15, 2024 16:55

Merge branch 'master' into type_request

c6d1917

chore: try fixing missing coverage

43b959d

CaselIT mentioned this pull request Aug 20, 2024

feat(typing): type App classes #2286

Merged

CaselIT and others added 7 commits August 20, 2024 22:44

Merge branch 'master' into type_request

d54a750

# Conflicts: # falcon/request.py

chore: remove no longer needed except clauses

59c32f6

chore: try making coverage happy

35702f0

chore: try making coverage happy, take 2

c0ac7be

Merge branch 'master' into type_request

306b376

# Conflicts: # falcon/typing.py # falcon/util/uri.py

typing: normalize naming convention

576c853

Merge branch 'master' into type_request

dcc7bff

vytas7 mentioned this pull request Aug 23, 2024

Some more typing for Request/Response classes #2205

Closed

vytas7 added 2 commits August 23, 2024 07:17

Merge branch 'master' into type_request

c8c05fe

chore: ruff check fixes

15becb9

vytas7 reviewed Aug 23, 2024

View reviewed changes

falcon/media/multipart.py Show resolved Hide resolved

Merge branch 'master' into type_request

72a2c95

vytas7 requested changes Aug 25, 2024

View reviewed changes

falcon/constants.py Outdated Show resolved Hide resolved

falcon/util/uri.py Show resolved Hide resolved

refactor: move missing to typing module; underscore it

9f8231b

CaselIT requested a review from vytas7 August 25, 2024 21:06

vytas7 reviewed Aug 26, 2024

View reviewed changes

vytas7 requested changes Aug 26, 2024

View reviewed changes

vytas7 approved these changes Aug 26, 2024

View reviewed changes

vytas7 merged commit 0bd3dc2 into falconry:master Aug 26, 2024
37 checks passed

CaselIT deleted the type_request branch August 26, 2024 09:18

vytas7 mentioned this pull request Aug 26, 2024

Privatize helpers, etc. that really should only be used internally by the framework #1457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(typing): type request module #2271

feat(typing): type request module #2271

CaselIT commented Aug 14, 2024 •

edited by vytas7

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

CaselIT left a comment

CaselIT Aug 14, 2024

copalco Aug 15, 2024

CaselIT Aug 14, 2024

CaselIT Aug 14, 2024

CaselIT Aug 14, 2024

CaselIT commented Aug 15, 2024

vytas7 commented Aug 15, 2024 •

edited

Loading

CaselIT commented Aug 15, 2024

CaselIT commented Aug 24, 2024

vytas7 left a comment

CaselIT commented Aug 25, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

vytas7 Aug 26, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

vytas7 Aug 26, 2024

CaselIT Aug 26, 2024

feat(typing): type request module #2271

feat(typing): type request module #2271

Conversation

CaselIT commented Aug 14, 2024 • edited by vytas7 Loading

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

CaselIT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaselIT commented Aug 15, 2024

vytas7 commented Aug 15, 2024 • edited Loading

CaselIT commented Aug 15, 2024

CaselIT commented Aug 24, 2024

vytas7 left a comment

Choose a reason for hiding this comment

CaselIT commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaselIT commented Aug 14, 2024 •

edited by vytas7

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

vytas7 commented Aug 15, 2024 •

edited

Loading