-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[multicast_dns] Allow decode of binary encoded TXT #5856
base: main
Are you sure you want to change the base?
[multicast_dns] Allow decode of binary encoded TXT #5856
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Side note, the decoding algorithm for TXT data is still not exactly in spec (see RFC6763 6.3-6.5). In particular:
These don't matter to me too much at the present time (and I have made no changes in this PR regarding them). It does look like the openthread border router's implementation has these restrictions on the '=' character within the key. |
From triage: ping @dnfield on this review |
@@ -338,11 338,15 @@ class TxtResourceRecord extends ResourceRecord { | |||
const TxtResourceRecord( | |||
String name, | |||
int validUntil, { | |||
required this.text, | |||
required this.attrs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay in this review.
I believe this would be a breaking change to TxtResourceRecord?
How about a new named constructor:
TxtResourceRecord.attributes(
String name,
int validUntil, {
required List<String> attributes,
}) : this(name, validUntil, text: attributes.join('\n'));
Also total nit but attributes
instead of attrs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A way to do this without it being a breaking change would be to:
- Make
text
nullable and mark it deprecated. - Add
attrs
as nullable. - Throw if (a) both or (b) neither are null.
After the deprecation period, mark attrs
as non-nullable and remove text
.
If very few packages are relying on this, then it might not be worth the effort.
Hi @ajhemphill91, have you been able to circle back to the requested change for the named constructor? Appreciate your contribution! |
Sorry for the delay, I have been on another project recently. If you do have a preference to keep the original constructor (and thus the original The Actually now that I am looking at it, |
From triage: @ajhemphill91 Are you still planning on updating this PR? |
I can apply the nit, but changing back to a named constructor that joins the records with a newline nullifies this PR in the first place. What would you like me to do? |
Sorry, didn't realize this PR was waiting for my response.
Hm, yeah I can't think of a way to do this without something unpleasant like a TxtResourceRecord(
String name,
int validUntil, {
required this.text,
}) : attrs = text.split('\n'),
super(ResourceRecordType.text, name, validUntil);
TxtResourceRecord.attributes(
String name,
int validUntil, {
required this.attrs,
}) : text = attrs.join('\n'),
super(ResourceRecordType.text, name, validUntil); Adding @cbracken for his opinion here. Ideally this could be done without a breaking change. |
I looked at the usage graph for |
@ajhemphill91 I've re-added the checklist from the template to the PR description; in the future please don't remove it. This PR is missing several required steps (notably for this discussion, a version change). |
Per the versioning guide should I then update the pubspec.yaml version to 0.4.0? I can take care of that and the CHANGELOG.md if everyone is okay with the breaking change. |
@jmagman Are you okay with the breaking change? |
LGTM, I can't remember if this will impact the tool but it would be easy to fix up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me. Suggestion on a non-breaking way to do this in my comment above.
@@ -338,11 338,15 @@ class TxtResourceRecord extends ResourceRecord { | |||
const TxtResourceRecord( | |||
String name, | |||
int validUntil, { | |||
required this.text, | |||
required this.attrs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A way to do this without it being a breaking change would be to:
- Make
text
nullable and mark it deprecated. - Add
attrs
as nullable. - Throw if (a) both or (b) neither are null.
After the deprecation period, mark attrs
as non-nullable and remove text
.
If very few packages are relying on this, then it might not be worth the effort.
@ajhemphill91 Looks like Dart formatting and CHANGELOG format checks are failing. See the log here for details: Once those are done, I leave it up to you as to whether you want to do the multi-step dance to go through the deprecation, then breaking change removal, or just go straight to the breaking change and land this as-is. |
Follow up on flutter/flutter#141218. Added a unit test with a binary-encoded payload.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.