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

[deep link] Add sub checks for ios domain error - format error #8309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hangyujin
Copy link
Contributor

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR.

Please add a note to packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md if your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or there is a reason for not adding tests.

build.yaml badge

If you need help, consider asking for help on Discord.

@hangyujin hangyujin requested a review from a team as a code owner September 7, 2024 00:57
@hangyujin hangyujin requested review from bkonyi and kenzieschmoll and removed request for a team and bkonyi September 7, 2024 00:57
final List<AASAfileFormatSubCheck> subcheckErrors;
}

IosDomainError iosFileFormatDomainError({
Copy link
Member

Choose a reason for hiding this comment

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

can this be a static method on IosDomainError for consistency with how the other errors are defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thanks!


static const appLinksFormat = AASAfileFormatSubCheck(
'Applinks format',
'This test checks if the `applinks` property holds an object.',
Copy link
Member

Choose a reason for hiding this comment

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

nit: in all the error explanations above, we use the verbiage "checks that" instead of "checks if". Let's use "checks that" where applicable in these errors for consistency

Comment on lines 235 to 238
'This test checks for `applinks.details.default`. Ref - '
'https://developer.apple.com/documentation/bundleresources/applinks/details/default'
'This checks if the `Defaults` property only holds `caseSensitive` and'
'`percentEncoded`.',
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above for the applinks.defaults property


static const componentPathFormat = AASAfileFormatSubCheck(
'Applinks details components path format',
'This test checks if the `applinks.details.components.path` property only holds a string.',
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should be consistent with whether we capitalize "s" in the word string throughout these errors

Copy link
Contributor Author

@hangyujin hangyujin Sep 13, 2024

Choose a reason for hiding this comment

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

good catch!

resolve comments

lint

lint

add tests

Update deep_links_services.dart

Update deep_links_services.dart

UI

Update deep_links_model.dart

add subchecks

Update deep_links_model.dart

1
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.

2 participants