Skip to content

Comments

Reorganize hooks for standards#237

Merged
annevk merged 2 commits intomasterfrom
annevk/hooks-for-standards-refactor
Oct 22, 2020
Merged

Reorganize hooks for standards#237
annevk merged 2 commits intomasterfrom
annevk/hooks-for-standards-refactor

Conversation

@annevk
Copy link
Member

@annevk annevk commented Oct 21, 2020

@andreubotella would appreciate your thoughts on this as well. I wanted to split this section into two given that I need two new algorithms for the URL standard (get an encoder and encode or fail) that both are somewhat wonky and probably need a bit of explanation and it seems bad to explain that together with all the UTF-8-centric primitives everyone ought to be using.


Preview | Diff

@annevk annevk requested a review from hsivonen October 21, 2020 10:19
Copy link
Member

@hsivonen hsivonen left a comment

Choose a reason for hiding this comment

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

I take it that the new hook will be in a different PR.

Copy link
Member

@andreubotella andreubotella left a comment

Choose a reason for hiding this comment

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

LGTM

@andreubotella
Copy link
Member

Since #238 has "encode or fail" passing an encoder instance (rather than an encoder "class", so to speak) to "run", it's probably best to update that algorithm in this PR to take an instance and to change the hooks that call it to use "run a new instance of encoding's decoder/encoder with..."

@annevk
Copy link
Member Author

annevk commented Oct 22, 2020

Let's make those changes there if we keep that as initially proposed. I rather like putting the errors directly in the queue though.

@annevk annevk merged commit cfc443e into master Oct 22, 2020
@annevk annevk deleted the annevk/hooks-for-standards-refactor branch October 22, 2020 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants