Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

WoI: chunk docs #4122

Merged
merged 2 commits into from
Oct 29, 2021
Merged

WoI: chunk docs #4122

merged 2 commits into from
Oct 29, 2021

Conversation

jaseweston
Copy link
Contributor

No description provided.

def message_mutation(self, message: Message) -> Message:

new_message = message.copy()
if '__retrieved-docs__' not in message:
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest replacing the hard-coded key values with the names from here

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: also this if condition could go before the deep copy (slight speed up).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

new_message = message.copy()
if '__retrieved-docs__' not in message:
return message
del new_message['__retrieved-docs__']
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this del necessary given that it gets set later in line 726?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you either have to do this or a force_set on the message I think..

Copy link
Contributor

@mojtaba-komeili mojtaba-komeili left a comment

Choose a reason for hiding this comment

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

Some very minor comments, but LGTM.

@jaseweston jaseweston merged commit ae5f14e into main Oct 29, 2021
@jaseweston jaseweston deleted the k2r1 branch October 29, 2021 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants