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.

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

Fixed #22712 -- Avoided name shadowing of "all" in findstatic command. #18259

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

avallbona
Copy link
Contributor

@avallbona avallbona commented Jun 8, 2024

Fixed TICKET-22712 - Consider not using built-in functions as parameters.

Trac ticket number

ticket-22712

Branch description

Replaced the name that shadows the builtin funcion all and replaced by fetch_all. I'm not sure if we need the changes to be backwards compatible.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes. (N/A)

@avallbona avallbona force-pushed the ticket-22712 branch 2 times, most recently from d3338d7 to 5d3e09b Compare June 8, 2024 18:50
@avallbona avallbona changed the title Fixed TICKET-22712 - Consider not using built-in functions as parameters. TICKET-22712 - Renamed the param name from **all** to **fetch_all**, in order to avoid shadowing the builtin function **all**. Jun 8, 2024
@avallbona avallbona changed the title TICKET-22712 - Renamed the param name from **all** to **fetch_all**, in order to avoid shadowing the builtin function **all**. TICKET-22712 - Consider not using built-in functions as parameters Jun 8, 2024
@avallbona avallbona force-pushed the ticket-22712 branch 5 times, most recently from 4a27321 to eabedf1 Compare June 10, 2024 22:24
@avallbona avallbona marked this pull request as ready for review June 11, 2024 06:49
@nessita nessita changed the title TICKET-22712 - Consider not using built-in functions as parameters Fixed #22712 -- Avoided name shadowing of "all" in findstatic command. Jun 20, 2024
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you @avallbona for this work! It looks good 🌟

I have added some comments and I also think that we should include release notes in the 5.2.txt file, under the section Features deprecated in 5.2. Something like:

* The ``all`` argument for the ``django.contrib.staticfiles.find()`` function
  is deprecated in favor of the ``find_all`` argument.

django/contrib/staticfiles/finders.py Outdated Show resolved Hide resolved
django/contrib/staticfiles/finders.py Outdated Show resolved Hide resolved
@avallbona
Copy link
Contributor Author

Thank you @avallbona for this work! It looks good 🌟

I have added some comments and I also think that we should include release notes in the 5.2.txt file, under the section Features deprecated in 5.2. Something like:

* The ``all`` argument for the ``django.contrib.staticfiles.find()`` function
  is deprecated in favor of the ``find_all`` argument.

Addressed the comments in 704fdb92b513ee0d6218068990bd70afa90dfdfd Thanks for the feedback 🙇🏿‍♂️

@avallbona avallbona requested a review from nessita June 20, 2024 21:45
@nessita
Copy link
Contributor

nessita commented Jun 21, 2024

Thank you @avallbona for the param rename! Looks great.

Did you see my comment about kwargs? Would you have some time to do some follow up with that?

@avallbona
Copy link
Contributor Author

Thank you @avallbona for the param rename! Looks great.

Did you see my comment about kwargs? Would you have some time to do some follow up with that?

I've answered in the comment about kwargs. Thanks for the feedback 🙇🏿‍♂️

@nessita
Copy link
Contributor

nessita commented Jun 26, 2024

Thank you @avallbona for the param rename! Looks great.
Did you see my comment about kwargs? Would you have some time to do some follow up with that?

I've answered in the comment about kwargs. Thanks for the feedback 🙇🏿‍♂️

Thank you, overall that looks good. I think that some comments such as:

# RemovedInDjango60Warning: When the deprecation ends, replace with:
# def find(self, path, find_all=False):

As a reference/example, we are just discussing/fixing a similar-ish issue for the save and asave methods. You can see how that was done in these PRs:

#17667
#18304
#18313

@avallbona
Copy link
Contributor Author

Thank you @avallbona for the param rename! Looks great.
Did you see my comment about kwargs? Would you have some time to do some follow up with that?

I've answered in the comment about kwargs. Thanks for the feedback 🙇🏿‍♂️

Thank you, overall that looks good. I think that some comments such as:

# RemovedInDjango60Warning: When the deprecation ends, replace with:
# def find(self, path, find_all=False):

As a reference/example, we are just discussing/fixing a similar-ish issue for the save and asave methods. You can see how that was done in these PRs:

#17667 #18304 #18313

@nessita handled in 3563132ea47cb8a0a52b2558e541bbc579535c30 Thanks for the feedback 🙇🏿‍♂️

@avallbona avallbona force-pushed the ticket-22712 branch 2 times, most recently from c1a1998 to f7a73ce Compare June 27, 2024 21:58
…ticfiles.finders.

Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
Copy link
Contributor

@nessita nessita left a comment

Choose a reason for hiding this comment

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

Thank you so much @avallbona for this work 🌟

I pushed a slight rework of the solution because, once I branched this PR to complete the review, I realized that the duplication for the deprecation check could be unified in a helper method.

I also took the opportunity to try the kwargs approach which I think worked well enough. Let me know what you think!

@nessita nessita merged commit 0fdcf10 into django:main Jun 28, 2024
35 checks passed
@nessita
Copy link
Contributor

nessita commented Jun 28, 2024

Deprecation timeline was corrected in #18321.

@avallbona
Copy link
Contributor Author

@nessita Thanks for your support along my first code contribution. The result of the fix I think it's satisfactory and I would have liked to had the chance to fix what @felixxm has fixed.

@nessita
Copy link
Contributor

nessita commented Jun 28, 2024

@nessita Thanks for your support along my first code contribution. The result of the fix I think it's satisfactory and I would have liked to had the chance to fix what @felixxm has fixed.

Right, this was my mistake: I should have noticed the deprecation number mismatch and I should have let you know so you could correct it. I'm sorry!

(In my head, I was still on 5.1 due to the recent beta release so I mixed the target deprecation version.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants