Did a timing analysis onrequest.data similar to the one madehere, and observed a time increase.

Timingsrequests.data
before0.022 s
after0.106 s

Good call running the timings. I didn't realize a custom property could cause that much of a slow down. That's good to know. I'll make the necessary updates.

I don't think that@safe_property should be used on every property. Usingwith wrap_attributeerrors: to thedata should be sufficient IMO.

As for which properties need it, it could definitely be dropped for some of the simple properties likecontent_type. However, any property that runs_load_data_and_files, which would includedata,POST, andFILESis susceptible to the original issue. If we want to drop the property, I could just explicitly add the context manager to those.

I think the custom property is useful, as it creates a repeatable pattern for how properties can be safely created inRequest class without having to manage the pitfalls of using__getattr__ andproperty together for each property individually, but it's not worth it if it slows things down so much.

@reddytocode Thank you for the review. I've dropped thesafe_property decorator and updated the properties that call_load_data_and_files to use thewrap_attributeerrors contextmanager.

I also moved the unit test under theTestContentParsing class.

Please let me know if any additional updates are needed

pls squash.

Done!

pls squash.

Just in case you weren't aware, Github allows projects to change the merge strategies used for PRs. For some of the projects I maintain, we've disabled merge methods other than squash, making it so all PR branches are automatically squashed when merged. You can configure it to pull the squashed commit message from the PR title and description too. It's pretty handy. I definitely recommend checking it out if you guys haven't and if this is a common requirement for PRs.

I know the feature, but thanks for pointing it out. The reason for my squashing request was rather that the previous commits make changes that were undone one or two commits later, and it was difficult to review.

Oh, interesting. Do you typically review PRs by looking at individual commits rather than the overall diff? I typically just look at the overall diff, but I'm wondering if I should start paying closer attention to individual commits as well.

Also, please let me know if any additional changes are needed.@auvipy has approved, but it looks like your request for changes is still preventing the PR from being merged. Please let me know if additional changes are needed besides squashing the commits.

Oh, interesting. Do you typically review PRs by looking at individual commits rather than the overall diff?

Some PRs are larger, and some people organize them such that each commit is a mental block, and that aids comprehension during reviewing things one by one. It kinda works if a commit contains to mental block (e.g. a test and a docs change), but it does not work so well if one commit undoes changes of a previous one.

Also, reviewing the combination only allows sneaking unreviewed code into the codebase (although not at the HEAD of the branch), and if the merge doesn't squash (which I don't usually assume), then those unreviewed commits show up in the main branch history.

For these two reasons, I like to review commits explicitly. But I get it that that's a matter of taste, and others may do it differently.

@auvipy has approved, but it looks like your request for changes is still preventing the PR from being merged.

Sorry! Approved.