You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks!
Multiple publications is a legitimate use case and the core of this is right as far as I'm concerned.
I'd like a small design pass before merging, though:
Display doing SQL escaping. right now publication.to_string() yields the SQL-escaped form (e.g. pub''a). Display is meant to be human-readable, so this would produce confusing output if the config is ever logged. could we keep Display showing the natural pub1,pub2 and move the quote-escaping back to the SQL call site (or a dedicated to_sql_literal() method)? escaping reads more clearly next to the query construction anyway.
Ergonomics for the multi-publication case. the feature's whole point is >1 publication, but the only way to build that is FromIterator (["a","b"].into_iter().collect::<Publication>()), while new() and the struct field take impl Into<Publication> and there's no From<Vec<String>> / From<Vec<&str>> / From<[&str; N]>, so a collection can't be passed naturally. could we add those From impls (and maybe a .publications([...]) builder) so the common case is discoverable? It's extra work but I think it's worth it! :)
Publication is opaque. private field, no constructor, no accessor , but it's a public type in a public field, so users can't read back what they set. A pub fn new, as_slice()/iter(), or similar would help.
Empty list.Publication built from an empty iterator produces publication_names '', which Postgres rejects (i think) at stream start. Might be worth rejecting empty up front with a clear error.
Tests. Could you add a test covering 2+ publications and the quote-escaping behavior?
Also note: like #8, the publication field type change is breaking (struct-literal users + anyone reading config.publication as String), so this is a 0.4.0-type change. And heads-up that this conflicts with #8 in start_replication - if we land #8 first, this'll need a rebase.
This is a nice addition overall, I'd just like the API to be a bit more ergonomic and the escaping decoupled from Display first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Might not be a super common case, but it is one I have.