please can you tell me more about this test, i’d like to understand why my pull request fails. on my local site, all the tests work.
So this is for your PR here: Added #34277 -- Add Conditional WHERE Clause to bulk_create for SQLite and PostgreSQL by HamaBarhamou · Pull Request #17515 · django/django · GitHub
If you click through to the details
on the docs build you can see a spelling error:
WARNING: ref/models/querysets.txt:2487: : Spell check: whereasin: whereasin PostgreSQL, you can utilize PostgreSQL-specific syntax and functions..
WARNING: Found 1 misspelled words
So the question is whereasin
a word? My quick search in the dictionary suggests whereas
is a word, but ``whereasinis not, so rephrasing to
whereas in` should solve your issue.
Kind Regards,
Carlton
Edit:
You can run make spelling
in the docs folder if you have the dependencies installed.
@carltongibson, I believe I have completed the modifications for my recent pull request. I eagerly await your feedback and suggestions for any necessary improvements. Thank you for your support and collaboration
Glad you got the build fixed there! — It looks like you’ve already got several people involved on the review, so I’ll leave it to them at this point.
Tank you. i hope not to wait forever
I see the Needs documentation flag is set on the ticket
https://code.djangoproject.com/ticket/34277
If you think you’ve addressed all the comments then you can uncheck that and it will appear on the Patches needing review list.
ok thanks for the info. i’ll uncheck it. i’ve already updated the documentation.
Hi @carltongibson . I still haven’t heard back, is that normal?
@HamaBarhamou is it normal that no-one re-reviewed your PR over the weekend? Yes, quite normal.
You have to understand that Django largely runs on volunteer effort (like your own). There are two paid Fellows, but they have a lot to do, and limited capacity.
Currently there are 18 tickets on the review queue of which yours is only one. Someone will get to it, but you’ll need to be patient.
The best thing to do is to make sure you’ve really addressed all the issues, so that it’s in the best state it can be when someone does get to review it.
If then you’re at a loose end, you can use the Patch review checklist to look at one of the other tickets if you like!
Kind Regards,
Carlton
super. Thank you, your answer is reassuring. I’m working on #5929 in the meantime.
Hi @HamaBarhamou. I’ve been reviewing your pull request for conditional on conflict update. I’m a volunteer and not paid so reviews and updates come as time allows.
I reviewed again yesterday - there is quite a bit outstanding to do before I think this is ready for the attention of the fellows (unless they’re interested of course!)
Thank you for your efforts on this - don’t be disheartened by the time between updates. Some things take time to ensure that we get the right approach as, once released, issues can be harder to fix.
Hi @ngnpope, thank you for reviewing my pull request for conditional update on conflict. I greatly appreciate the time and effort you put into this, especially knowing that it’s on a volunteer and unpaid basis.
I understand that this process can take time, and I am completely prepared to work patiently on the necessary improvements. Your feedback is crucial to ensure that we are taking the right approach. I agree that issues found after publication can be more challenging to resolve, so let’s take the necessary time to get things right from the start.
To move forward, I have posted some clarification questions on GitHub and would be very grateful for your guidance or responses when you find the time. Your advice is invaluable in helping me understand and improve my contribution.
Thank you again for your support and guidance
hi @ngnpope , i did an update. i’m ready for another revision. thanks
merry christmas to you. @felixxm please I’m ready for another revision or maybe we should wait for the new year ?
https://github.com/django/django/pull/17515
Hi @sarahboyce , sorry there are enough posts and I’m now having trouble loading all the posts on github. however I have replied to all your previous comments. but I don’t know if you’ve seen them because of the “pending” mention that appears.
as far as versioning documentation is concerned, I was guided by @ngnpope 's comments.
For the documentation, it’s @felixxm '.
I don’t want to go round in circles on this question, because I have the impression that you’re not saying the same thing.
all that’s left is the question of documentation. i’d be delighted if @sarahboyce @felixxm and @ngnpope said the same thing.
As for the question of the Ecluded subclass of F, I solved the problem in my last commit. You may have missed it. Here’s the code:
class Excluded(F):
def resolve_expression(self, *args, **kwargs):
super().resolve_expression(*args, **kwargs)
self.output_field = None
return self
def as_sql(self, compiler, connection):
if not connection.features.supports_excluded_expression:
raise NotSupportedError(
f"{connection.vendor} doesn't support partial updating rows on "
"conflicts during INSERT"
)
quoted_field_name = compiler.quote_name_unless_alias(self.name)
sql = f"EXCLUDED.{quoted_field_name}"
return sql, []
def __repr__(self):
return f"{self.__class__.__name__}({self.name})"
@cached_property
def output_field(self):
"""Return the output type of this expressions."""
output_field = self._resolve_output_field()
if output_field is None:
self._output_field_resolved_to_none = True
raise FieldError("Cannot resolve expression type, unknown output_field")
return output_field
@cached_property
def _output_field_or_none(self):
"""
Return the output field of this expression, or None if
_resolve_output_field() didn't return an output type.
"""
try:
return self.output_field
except FieldError:
if not self._output_field_resolved_to_none:
raise
Ah so you have! I’m sorry Hama I missed that! Also I just received the comments now, having another look
thank you for your time
Ok, I only have open comments about the docs changes
You can keep the database flags to be in the backwards incompatible changes for now (I see that was a suggestion from Nick) but I think the model changes and Excluded should be in the Models section of the release notes similar to how the new create_defaults
argument was documented here: Django 5.0 release notes | Django documentation | Django
I would still make the doc updates I was suggesting I can’t tell if Mariusz wrote this for you (which I think you’re implying) but even if that’s the case, other people can still give feedback.
But I understand if you want to wait for input from Nick and Mariusz
yes indeed, thank you @sarahboyce for your feedback. @felixxm and @ngnpope I’d like your opinion on this before I start touching the documentation.
Also here is @felixxm 's comment here:
You should use
versionchanged
, notversionadded
. Alsoversion*
annotations are for small notes, not for the entire documentation of new features. You should change a signature in docs and document the new argument directly in thebulk_create()
docs.
unless I’ve misunderstood.
I don’t see anything in my recommendations that is against Sarah’s suggestions. Besides, I only left initial comments, I’d probably left many new comments in the next run.
In general, please stop pinging reviewers directly and leaving them direct messages. There is no need to ask for another review on all possible channels. You cannot expect immediate replies. You should read the following FAQ before you will consider doing this again:
Last but not least, don’t leave comments in French.