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

Fix: #1834. Remove incremental algorithms. #1848

Merged
merged 3 commits into from Dec 17, 2021
Merged

Fix: #1834. Remove incremental algorithms. #1848

merged 3 commits into from Dec 17, 2021

Conversation

ioggstream
Copy link
Contributor

@ioggstream ioggstream commented Dec 11, 2021

This PR

Fix: #1834. Remove incremental algorithms.

@LPardue tweak the text as you prefer and merge :)

@LPardue
Copy link
Contributor

LPardue commented Dec 13, 2021

The new text looks ok but since it only appears in the Representation Digest section, it gives the impression that it doesn't apply to content digest.

@ioggstream
Copy link
Contributor Author

@LPardue PTAL

When it is convenient to do so,
the sender and the receiver can dynamically compute the checksum value
while streaming the content.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an idea, what do you think about moving these 3 paragraphs to above the declaration of the registry? I could propose a follow up PR to this one if its easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LPardue Great, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it... PTAL

digest-algorithm = token
~~~

When it is convenient to do so,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see some indication of what would make it "convenient" here -- this is as unclear as "incremental" was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably it's better to mention incremental algorithms then :P

Suggested change
When it is convenient to do so,
When using algorithms based on incremental computation,
(see {{?RFC1624}})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alternatively just pass the ball to implementers.

Suggested change
When it is convenient to do so,
When implementers deem convenient to do so,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LPardue if none of the above solutions work, I'd just remove these 3 lines since checksum implementers already know that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

lets just delete it

@ioggstream ioggstream merged commit 3c6ce1f into main Dec 17, 2021
@ioggstream ioggstream deleted the ioggstream-1834 branch December 17, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Define or cite "incremental digest-algorithm"
3 participants