Skip to content

Adding a flag to devtools_tools serve for dtd uri - #7191

Merged
CoderDake merged 8 commits into
flutter:masterfrom
CoderDake:dtd-uri-flag
Feb 15, 2024
Merged

CoderDake merged 8 commits into
flutter:masterfrom
CoderDake:dtd-uri-flag

Conversation

@CoderDake

Copy link
Copy Markdown
Contributor

The devtools server is getting a --dtd-uri flag in https://dart-review.googlesource.com/c/sdk/+/346922

This change allows devtools_tool serve to pass along that flag.

@CoderDake
CoderDake marked this pull request as ready for review February 9, 2024 18:47
@CoderDake
CoderDake requested a review from a team as a code owner February 9, 2024 18:47
Comment thread packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md Outdated
kenzieschmoll
kenzieschmoll previously approved these changes Feb 9, 2024
@kenzieschmoll

Copy link
Copy Markdown
Member

Actually wait a minute. Since we are already forwarding remaining arguments through to the command to start the DevTools server, I don't think we need to parse this manually from the serve command. Try passing the --dtd-uri flag to the serve command without this change.

@kenzieschmoll
kenzieschmoll self-requested a review February 9, 2024 18:56
@kenzieschmoll
kenzieschmoll dismissed their stale review February 9, 2024 18:56

undo approval

@CoderDake

Copy link
Copy Markdown
Contributor Author

@kenzieschmoll without this change i got the following error:

Screenshot 2024-02-09 at 1 59 57 PM

So I figured that adding it here was kind of like allowlisting it

@DanTup

DanTup commented Feb 14, 2024 •

Copy link
Copy Markdown
Contributor

I think we do need this for the same reasons we have things like --allow-embed and --machine. The current arg parser allows for additional arguments, but not additional options/flags (unless we use AllowAnythingParser). (see discussion in #6670)

@kenzieschmoll kenzieschmoll left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Can you add a TODO to consider using AllowAnythingParser? Would also be good to separate the flags consts that are there to pass through to the DevTools server, and add a comment. (_dtdUriFlag, _machineFlag, _allowEmbeddingFlag) - any flag that we aren't removing later is intended to be passed through.

Comment thread tool/lib/commands/serve.dart Outdated
@CoderDake
CoderDake merged commit 670bd5e into flutter:master Feb 15, 2024
@CoderDake
CoderDake deleted the dtd-uri-flag branch February 15, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants