Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ Add new items at the end of the relevant section under **Unreleased**.

### Additions

- Add `validate` subcommand to generate validate reports ([#79])
- Add `--pull-request` option to validate proposals in a pull request ([#86])
- Add `validate` subcommand to generate validate reports ([#83])
- Add `--pull-request` option to validate proposals in a pull request ([#87])

### Changes

- Support extraction of new "Summary of changes" section ([#79])
- Errors and warnings now include unique error codes ([#90])
- Add generalized mechanism for validation exemptions ([#92])

### Fixes

Expand Down Expand Up @@ -84,4 +86,6 @@ This changelog's format is based on [Keep a Changelog](https://keepachangelog.co
[#74]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/74
[#79]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/79
[#83]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/83
[#86]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/86
[#87]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/87
[#90]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/90
[#92]: https://github.com/swiftlang/swift-evolution-metadata-extractor/pull/92
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,19 @@ struct DiscussionExtractor: MarkupWalker, ValueExtractor {
private var source: HeaderFieldSource
init(source: HeaderFieldSource) { self.source = source }

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
private var discussions: [Proposal.Discussion] = []

mutating func extractValue() -> ExtractionResult<[Proposal.Discussion]> {

// VALIDATION ENHANCEMENT: Normalize naming to 'Review' in the source proposals.
if let (_, headerField) = source["Review", "Reviews", "Decision Notes", "Decision notes"] {
visit(headerField)

// VALIDATION ENHANCEMENT: Correct proposals with known issues and remove special case logic.
// Currently a fair number of older proposals are missing links to discussions or do not
// format discussions correctly. Those issues should be corrected in the proposals themselves.
// Once all of those issues are resolved, the legacy check can be removed.
if discussions.isEmpty && !Legacy.discussionExtractionFailures.contains(source.proposalSpec.id) {
errors.append(.discussionExtractionFailure)
if discussions.isEmpty {
issues.reportIssue(.discussionExtractionFailure, source: source)
}
} else {
// VALIDATION ENHANCEMENT: Add field to proposals with missing field and remove special case logic.
// 'Review' is a required field, but currently a fair number of older proposals are missing the
// field for a variety of reasons. Those issues should be corrected in the proposals themselves.
// Once all of those issues are resolved, the legacy check can be removed.
// Note that some very early proposals may not have valid discussions be extracted.
if !Legacy.missingReviewFields.contains(source.proposalSpec.id) {
errors.append(.missingReviewField)
}
issues.reportIssue(.missingReviewField, source: source)
}

// VALIDATION ENHANCEMENT: Potentially check for:
Expand All @@ -52,15 +38,7 @@ struct DiscussionExtractor: MarkupWalker, ValueExtractor {
// - non-standard discussion names
// - Formatting (in parenthesis, separated by comma, etc.

return ExtractionResult(value: discussions, warnings: warnings, errors: errors)
}

// Existing proposals with known validation errors.
// The listed exceptions will not generate warnings and errors for the known issues.
private enum Legacy {
static let missingReviewFields: Set<String> = ["SE-0001", "SE-0002", "SE-0004", "SE-0020", "SE-0051", "SE-0079", "SE-0100", "SE-0176", "SE-0177", "SE-0188", "SE-0193", "SE-0194", "SE-0196", "SE-0198", "SE-0201", "SE-0203", "SE-0205", "SE-0208", "SE-0209", "SE-0210", "SE-0212", "SE-0213", "SE-0219", "SE-0243", "SE-0245", "SE-0247", "SE-0248", "SE-0249", "SE-0250", "SE-0252", "SE-0259", "SE-0263", "SE-0268", "SE-0269", "SE-0273", "SE-0278", "SE-0284", "SE-0289", "SE-0295", "SE-0300", "SE-0303", "SE-0304", "SE-0312", "SE-0313", "SE-0317", "SE-0318", "SE-0337", "SE-0338", "SE-0341", "SE-0343", "SE-0344", "SE-0348", "SE-0350", "SE-0356", "SE-0365", "SE-0385"]

static let discussionExtractionFailures: Set<String> = ["SE-0392"]
return ExtractionResult(value: discussions, warnings: issues.warnings, errors: issues.errors)
}

mutating func visitLink(_ link: Link) -> () {
Expand All @@ -73,7 +51,7 @@ struct DiscussionExtractor: MarkupWalker, ValueExtractor {
if let discussionURL = linkInfo.swiftForumsDestination {
discussions.append(Proposal.Discussion(name: linkInfo.text, link: discussionURL))
} else {
warnings.append(.invalidDiscussionLink)
issues.reportIssue(.invalidDiscussionLink, source: source)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ struct ImplementationExtractor: MarkupWalker, ValueExtractor {
private var source: HeaderFieldSource
init(source: HeaderFieldSource) { self.source = source }

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
private var implementaton: [Proposal.Implementation]? {
_implementaton.isEmpty ? nil : _implementaton
}
Expand All @@ -29,7 +27,7 @@ struct ImplementationExtractor: MarkupWalker, ValueExtractor {
}
// Implementation field is optional. Take no action / add no warning if it is not found

return ExtractionResult(value: implementaton, warnings: warnings, errors: errors)
return ExtractionResult(value: implementaton, warnings: issues.warnings, errors: issues.errors)
}


Expand Down Expand Up @@ -77,7 +75,7 @@ struct ImplementationExtractor: MarkupWalker, ValueExtractor {

// VALIDATION ENHANCEMENT: The legacy tool lowercases the tested value. This is probably not necessary.
guard account == "apple" || account == "swiftlang" else {
warnings.append(.invalidImplementationLink)
issues.reportIssue(.invalidImplementationLink, source: source)
return
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ struct PersonExtractor: MarkupWalker {
self.role = role
}

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
private var personList: [Proposal.Person] = []

mutating func personArray() -> ExtractionResult<[Proposal.Person]> {
Expand All @@ -57,7 +55,7 @@ struct PersonExtractor: MarkupWalker {

// VALIDATION ENHANCEMENT: Log a warning if no review manager. It should be a CI validation error also

return ExtractionResult(value: personList, warnings: warnings, errors: errors)
return ExtractionResult(value: personList, warnings: issues.warnings, errors: issues.errors)
}

mutating func visitLink(_ link: Link) -> () {
Expand All @@ -66,16 +64,16 @@ struct PersonExtractor: MarkupWalker {
}
// VALIDATION ENHANCEMENT: Add 'extra markup error' for review managers
if !linkInfo.containsTextElement && role == .author {
errors.append(.authorsHaveExtraMarkup)
issues.reportIssue(.authorsHaveExtraMarkup, source: source)
}

let destination: String
if let validatedDestination = linkInfo.gitHubDestination {
destination = validatedDestination
} else {
switch role {
case .author: warnings.append(.invalidAuthorLink)
case .reviewManager: warnings.append(.invalidReviewManagerLink)
case .author: issues.reportIssue(.invalidAuthorLink, source: source)
case .reviewManager: issues.reportIssue(.invalidReviewManagerLink, source: source)
}
destination = ""
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ struct PreviousProposalExtractor: MarkupWalker, ValueExtractor {
private var source: HeaderFieldSource
init(source: HeaderFieldSource) { self.source = source }

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
private var previousProposalIDs: [String]? {
_previousProposalIDs.isEmpty ? nil : _previousProposalIDs
}
Expand All @@ -29,11 +27,11 @@ struct PreviousProposalExtractor: MarkupWalker, ValueExtractor {

// validate that if the header field is here at least one proposal ID was found
if _previousProposalIDs.isEmpty {
errors.append(.previousProposalIDsExtractionFailure)
issues.reportIssue(.previousProposalIDsExtractionFailure, source: source)
}
}

return ExtractionResult(value: previousProposalIDs, warnings: warnings, errors: errors)
return ExtractionResult(value: previousProposalIDs, warnings: issues.warnings, errors: issues.errors)
}

mutating func visitText(_ text: Text) -> () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,47 +14,45 @@ struct ProposalLinkExtractor: MarkupWalker, ValueExtractor {
private var source: HeaderFieldSource
init(source: HeaderFieldSource) { self.source = source }

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
private var proposalLink: LinkInfo? = nil

mutating func extractValue() -> ExtractionResult<LinkInfo> {
if let headerField = source["Proposal"] {
visit(headerField)
} else {
errors.append(.missingProposalField)
issues.reportIssue(.missingProposalField, source: source)
}
if let proposalLink {

if !proposalLink.containsTextElement {
warnings.append(.proposalIDHasExtraMarkup)
issues.reportIssue(.proposalIDHasExtraMarkup, source: source)
}

// VALIDATION ENHANCEMENT: To match legacy behavior, if ID has extra markup, the
// ID is an empty string. An enhancement would be to capture the ID if possible
if !proposalLink.text.isEmpty {

if proposalLink.text == "SE-0000" {
errors.append(.reservedProposalID)
issues.reportIssue(.reservedProposalID, source: source)
}

if !proposalLink.text.contains(source.project.proposalRegex) {
self.proposalLink?.destination = "" // Do not include an incorrect destination
errors.append(.proposalIDWrongDigitCount)
issues.reportIssue(.proposalIDWrongDigitCount, source: source)
}

// The link should be relative and contain the correct number of digits.
if !proposalLink.destination.contains(/^\d\d\d\d-.*\.md$/) {
self.proposalLink?.destination = "" // Do not include an incorrect destination
errors.append(Proposal.Issue.invalidProposalIDLink)
issues.reportIssue(Proposal.Issue.invalidProposalIDLink, source: source)
}
}

} else {
errors.append(.missingProposalIDLink)
issues.reportIssue(.missingProposalIDLink, source: source)
}
return ExtractionResult(value: proposalLink ?? LinkInfo(text: "", destination: ""), warnings: warnings, errors: errors)
return ExtractionResult(value: proposalLink ?? LinkInfo(text: "", destination: ""), warnings: issues.warnings, errors: issues.errors)
}

mutating func visitLink(_ link: Link) -> () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ struct StatusExtractor: MarkupWalker, ValueExtractor {
self.extractionDate = source.extractionDate
}

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
var status: Proposal.Status? = nil

mutating func extractValue() -> ExtractionResult<Proposal.Status> {
Expand All @@ -33,9 +31,9 @@ struct StatusExtractor: MarkupWalker, ValueExtractor {

// This checks both that the field is found and is successfully extracted
if status == nil {
warnings.append(.missingStatus)
issues.reportIssue(.missingStatus, source: source)
}
return ExtractionResult(value: status, warnings: warnings, errors: errors)
return ExtractionResult(value: status, warnings: issues.warnings, errors: issues.errors)
}

mutating func visitStrong(_ strong: Strong) -> () {
Expand All @@ -60,17 +58,17 @@ struct StatusExtractor: MarkupWalker, ValueExtractor {
start = result.start
end = result.end
if let warning = result.reviewEndedWarning {
warnings.append(warning)
issues.reportIssue(warning, source: source)
}
} else {
warnings.append(.missingOrInvalidReviewDates)
issues.reportIssue(.missingOrInvalidReviewDates, source: source)
}
}

if let rawStatus = Proposal.Status(name: statusString, version: version, start: start, end: end) {
status = rawStatus
} else {
errors.append(.missingOrInvalidStatus)
issues.reportIssue(.missingOrInvalidStatus, source: source)
status = .statusExtractionFailed
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ struct TitleExtractor: ValueExtractor {
private var source: DocumentSource
init(source: DocumentSource) { self.source = source }

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
mutating func extractValue() -> ExtractionResult<String> {

var title: String?
Expand All @@ -30,9 +28,9 @@ struct TitleExtractor: ValueExtractor {
title = titleElement.plainText

} else {
errors.append(.proposalContainsNoContent)
issues.reportIssue(.proposalContainsNoContent, source: source)
}
return ExtractionResult(value: title, warnings: warnings, errors: errors)
return ExtractionResult(value: title, warnings: issues.warnings, errors: issues.errors)

}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@ struct UpcomingFeatureFlagExtractor: MarkupWalker, ValueExtractor {
private var source: HeaderFieldSource
init(source: HeaderFieldSource) { self.source = source }

private var warnings: [Proposal.Issue] = []
private var errors: [Proposal.Issue] = []

private var issues = IssueWrapper()
private var flag: String?

private var uff: Proposal.UpcomingFeatureFlag? {
Expand All @@ -36,15 +34,15 @@ struct UpcomingFeatureFlagExtractor: MarkupWalker, ValueExtractor {
if let flag {
if flag.contains(/\s/) {
self.flag = nil
errors.append(.malformedUpcomingFeatureFlag)
issues.reportIssue(.malformedUpcomingFeatureFlag, source: source)
}
} else {
errors.append(.upcomingFeatureFlagExtractionFailure)
issues.reportIssue(.upcomingFeatureFlagExtractionFailure, source: source)
}

}

return ExtractionResult(value: uff, warnings: warnings, errors: errors)
return ExtractionResult(value: uff, warnings: issues.warnings, errors: issues.errors)
}

mutating func visitInlineCode(_ inlineCode: InlineCode) -> () {
Expand Down
Loading