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.

Sign up for GitHub

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

Jump to bottom

Fixed #35187 -- Fixed @sensitive_variables/sensitive_post_parameters decorators crash with .pyc-only builds. #17860

Merged
merged 1 commit into from Feb 17, 2024
Merged

Fixed #35187 -- Fixed @sensitive_variables/sensitive_post_parameters decorators crash with .pyc-only builds. #17860

merged 1 commit into from Feb 17, 2024

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Feb 16, 2024

Thanks Jon Janzen for the implementation idea.

Thanks Marcus Hoffmann for the report.

Regression in 38e391e.

ticket-35187

@felixxm
Copy link
Member Author

felixxm commented Feb 16, 2024

I don't see a reasonable (and valuable) way to test this. We could mock inspect.getsourcelines() to raise an exception, but since it's no longer used I don't see much value in such a test.

@felixxm
Fixed #35187 -- Fixed @sensitive_variables/sensitive_post_parameters …
33bf781 
…decorators crash with .pyc-only builds.

Thanks Jon Janzen for the implementation idea.

Thanks Marcus Hoffmann for the report.

Regression in 38e391e.
bigfootjon
bigfootjon approved these changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@bigfootjon bigfootjon left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think there’s a test case to be added for this change, but perhaps we should have a test of pyc only projects?

@felixxm felixxm requested a review from a team February 16, 2024 21:26
@felixxm
Copy link
Member Author

felixxm commented Feb 16, 2024

I don’t think there’s a rest case to be added for this change, but perhaps we should have a test of pyc only projects?

Do you think it's possible to add tests for .pyc-only Django to the daily builds (as we did for pypy, #17499)? Would you like to work on this?

@bigfootjon
Copy link
Contributor

bigfootjon commented Feb 17, 2024 •
edited

Do you think it's possible to add tests for .pyc-only Django to the daily builds (as we did for pypy, #17499)? Would you like to work on this?

I have no experience working with pyc-only projects but it sounds interesting to work on, so yes I can work on that. (probably sometime this weekend)

Do you want me to file a ticket?

carltongibson
carltongibson approved these changes Feb 17, 2024
View reviewed changes
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thanks all.

@felixxm
Copy link
Member Author

felixxm commented Feb 17, 2024

Thanks y'all for review 🚀

Do you want me to file a ticket?

Ticket is not necessary at this stage.

@felixxm felixxm merged commit d1be05b into django:main Feb 17, 2024
35 checks passed
@felixxm felixxm deleted the issue-35187 branch February 17, 2024 07:17
@bigfootjon bigfootjon mentioned this pull request Feb 17, 2024
Merged
@bigfootjon
Copy link
Contributor

@felixxm, I've put up #17870 to add the daily pyc-only job. Please review at your convenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charettes charettes charettes approved these changes

@carltongibson carltongibson carltongibson approved these changes

@bigfootjon bigfootjon bigfootjon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@felixxm @bigfootjon @charettes @carltongibson