Skip to content

Conversation

jonasfj
Copy link
Member

@jonasfj jonasfj commented Dec 7, 2023

See #7263

@jonasfj jonasfj requested a review from szakarias December 7, 2023 17:26
Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

We should probably link to this file from documentation somewhere.

Maybe even from https://pub.dev/topics ...


/// True, if [topic] is formatted like a valid topic.
bool isValidTopicFormat(String topic) =>
RegExp(r'^[a-z0-9-]{2,32}$').hasMatch(topic) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we not define a regexp for this somewhere else in upload-validation code, should probably be shared?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it lives in pkg/pub_package_reader, I think this is so trivial I just duplicated it rather than add another dependency.

We can change it if we want to. This is really just to test the configuration file.

Copy link
Member Author

Choose a reason for hiding this comment

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

It has a Iterable<ArchiveIssue> checkTopics(String pubspecContent) function, which IMO should be private (or in src/) because the only thing pub_package_reader should expose is Future<PackageSummary> summarizePackageArchive(...).

So even if there was a good method we did export that we could use, I think it would be wrong to do so. pub_package_reader is for reading packages and validating them.

One could argue we should have a separate package or library somewhere called topic_validation.dart -- but we don't, and I'm not sure where it should live :D

jonasfj and others added 4 commits December 11, 2023 10:10
Co-authored-by: Sigurd Meldgaard <[email protected]>
Co-authored-by: Sigurd Meldgaard <[email protected]>
Co-authored-by: Sigurd Meldgaard <[email protected]>
Copy link
Contributor

@szakarias szakarias left a comment

Choose a reason for hiding this comment

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

LGTM

@jonasfj jonasfj merged commit 1478edc into dart-lang:master Dec 11, 2023
@jonasfj jonasfj deleted the topics-yaml branch December 11, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants