diff --git a/CHANGELOG.md b/CHANGELOG.md index e1ab0fc..6c807a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 @@ -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 diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/DiscussionExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/DiscussionExtractor.swift index 9805e21..537c2ea 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/DiscussionExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/DiscussionExtractor.swift @@ -14,9 +14,7 @@ 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]> { @@ -24,23 +22,11 @@ struct DiscussionExtractor: MarkupWalker, ValueExtractor { // 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: @@ -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 = ["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 = ["SE-0392"] + return ExtractionResult(value: discussions, warnings: issues.warnings, errors: issues.errors) } mutating func visitLink(_ link: Link) -> () { @@ -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) } } } diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ImplementationExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ImplementationExtractor.swift index b88c7fb..88855e9 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ImplementationExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ImplementationExtractor.swift @@ -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 } @@ -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) } @@ -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 } diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PersonExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PersonExtractor.swift index b0a50e7..c06b975 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PersonExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PersonExtractor.swift @@ -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]> { @@ -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) -> () { @@ -66,7 +64,7 @@ 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 @@ -74,8 +72,8 @@ struct PersonExtractor: MarkupWalker { 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 = "" } diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PreviousProposalExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PreviousProposalExtractor.swift index b438276..7833cbd 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PreviousProposalExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/PreviousProposalExtractor.swift @@ -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 } @@ -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) -> () { diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ProposalLinkExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ProposalLinkExtractor.swift index 046cf47..ac9070c 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ProposalLinkExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/ProposalLinkExtractor.swift @@ -14,21 +14,19 @@ 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 { 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 @@ -36,25 +34,25 @@ struct ProposalLinkExtractor: MarkupWalker, ValueExtractor { 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) -> () { diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/StatusExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/StatusExtractor.swift index 072e190..55219c2 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/StatusExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/StatusExtractor.swift @@ -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 { @@ -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) -> () { @@ -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 } } diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/TitleExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/TitleExtractor.swift index 9d6d751..ddaf1dd 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/TitleExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/TitleExtractor.swift @@ -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 { var title: String? @@ -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) } } diff --git a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/UpcomingFeatureFlagExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/UpcomingFeatureFlagExtractor.swift index 077918e..816614f 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/UpcomingFeatureFlagExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/FieldExtractors/UpcomingFeatureFlagExtractor.swift @@ -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? { @@ -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) -> () { diff --git a/Sources/EvolutionMetadataExtraction/Extractors/ProposalMetadataExtractor.swift b/Sources/EvolutionMetadataExtraction/Extractors/ProposalMetadataExtractor.swift index 6c3c3db..d8d23ec 100644 --- a/Sources/EvolutionMetadataExtraction/Extractors/ProposalMetadataExtractor.swift +++ b/Sources/EvolutionMetadataExtraction/Extractors/ProposalMetadataExtractor.swift @@ -19,18 +19,15 @@ struct ProposalMetadataExtractor { /// - extractionDate: Extraction date used to determine expired review periods /// - Returns: The extracted proposal metadata static func extractProposalMetadata(from markdown: String, proposalSpec: ProposalSpec, extractionDate: Date) -> Proposal { - + + var issues = IssueWrapper() var proposal = Proposal() proposal.sha = proposalSpec.sha - - var warnings: [Proposal.Issue] = [] - var errors: [Proposal.Issue] = [] - + func extractValue(from source: T.Source, with extractorType: T.Type) -> T.ResultValue? { var extractor = extractorType.init(source: source) let result = extractor.extractValue() - warnings.append(contentsOf: result.warnings) - errors.append(contentsOf: result.errors) + issues.append(errors: result.errors, warnings: result.warnings) return result.value } @@ -58,13 +55,13 @@ struct ProposalMetadataExtractor { if let authors = extractValue(from: headerFieldsSource, with: AuthorExtractor.self), !authors.isEmpty { proposal.authors = authors } else { - errors.append(.missingAuthors) + issues.reportIssue(.missingAuthors, source: headerFieldsSource) } if let reviewManagers = extractValue(from: headerFieldsSource, with: ReviewManagerExtractor.self), !reviewManagers.isEmpty { proposal.reviewManagers = reviewManagers } else { - warnings.append(.missingReviewManagers) + issues.reportIssue(.missingReviewManagers, source: headerFieldsSource) } proposal.upcomingFeatureFlag = extractValue(from: headerFieldsSource, with: UpcomingFeatureFlagExtractor.self) @@ -75,7 +72,7 @@ struct ProposalMetadataExtractor { if let discussions = extractValue(from: headerFieldsSource, with: DiscussionExtractor.self) { proposal.discussions = discussions } else { - errors.append(.missingReviewField) + issues.reportIssue(.missingReviewField, source: headerFieldsSource) } if let status = extractValue(from: (headerFieldsSource, extractionDate), with: StatusExtractor.self) { @@ -96,12 +93,12 @@ struct ProposalMetadataExtractor { proposal.status = .statusExtractionFailed } } else { - errors.append(.missingMetadataFields) + issues.reportIssue(.missingMetadataFields, source: documentSource) } - // Add warnings and errors if present - if !warnings.isEmpty { proposal.warnings = warnings } - if !errors.isEmpty { proposal.errors = errors } + // Add warnings and errors to proposal if present + if !issues.warnings.isEmpty { proposal.warnings = issues.warnings } + if !issues.errors.isEmpty { proposal.errors = issues.errors } return proposal } @@ -176,7 +173,26 @@ struct LinkInfo { } } -// Protocol and structs to encapsulate source information for extractors +// Protocol and structs to encapsulate source and issue information for extractors + +struct IssueWrapper { + private(set) var errors: [Proposal.Issue] = [] + private(set) var warnings: [Proposal.Issue] = [] + mutating func reportIssue(_ issue: Proposal.Issue, source: Source) { + if let exclusions = source.project.validationExemptions[issue.code], + exclusions.contains(source.proposalNumber) { return } + else { + switch issue.kind { + case .error: errors.append(issue) + case .warning: warnings.append(issue) + } + } + } + mutating func append(errors: [Proposal.Issue], warnings: [Proposal.Issue]) { + self.errors.append(contentsOf: errors) + self.warnings.append(contentsOf: warnings) + } +} protocol IssueSource { var project: Project { get } diff --git a/Sources/EvolutionMetadataExtraction/Utilities/Project.swift b/Sources/EvolutionMetadataExtraction/Utilities/Project.swift index 283bbca..a73bfc1 100644 --- a/Sources/EvolutionMetadataExtraction/Utilities/Project.swift +++ b/Sources/EvolutionMetadataExtraction/Utilities/Project.swift @@ -7,6 +7,10 @@ // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors import Foundation +import EvolutionMetadataModel + +// typealias improves readability of validationExemptions definitions +fileprivate typealias Issue = Proposal.Issue /** Represents a software project with its own set of evolution proposals. @@ -59,7 +63,21 @@ public final class Project: Sendable { proposalRegex: /^SE-\d\d\d\d$/, previousResultsURL: URL(string: "https://download.swift.org/swift-evolution/v1/evolution.json")!, defaultOutputFilename: "evolution.json", - validationExemptions: [:] + validationExemptions: [ + + // '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 corrected, the exemption for the corrected proposal can be removed. + // Note that some early proposals may not have valid discussions be added and will always need exemption. + Issue.missingReviewField.code: + RangeSet([0001, 0002, 0004, 0020, 0051, 0079, 0100, 0176, 0177, 0188, 0193, 0194, 0196, 0198, 0201, 0203, 0205, 0208, 0209, 0210, 0212, 0213, 0219, 0243, 0245, 0247, 0248, 0249, 0250, 0252, 0259, 0263, 0268, 0269, 0273, 0278, 0284, 0289, 0295, 0300, 0303, 0304, 0312, 0313, 0317, 0318, 0337, 0338, 0341, 0343, 0344, 0348, 0350, 0356, 0365, 0385]), + + // Some older proposals are missing links to discussions or do not format discussions correctly. + // Those issues should be corrected in the proposals themselves. + // Once corrected, the exemption for the corrected proposal can be removed. + Issue.discussionExtractionFailure.code: + RangeSet(0392), + ] ) public static let swiftTesting = Project( @@ -86,3 +104,27 @@ public final class Project: Sendable { validationExemptions: [:] ) } + +// MARK: - Range and RangeSet Extensions + +/* Extensions to Range and RangeSet to increase clarity of validationExemptions definitions */ + +private extension Range where Bound == Int { + static func upTo(_ upperBound: Int) -> Range { 0..