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

Lints to ensure link text for EIPs should match the EIP's number #99

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
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
12 changes: 12 additions & 0 deletions eipw-lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,12 @@ pub fn default_lints_enum() -> impl Iterator<Item = (&'static str, DefaultLint<&
message: "proposals must be referenced with the form `EIP-N` (not `EIPN` or `EIP N`)",
}),
),
(
"markdown-link-eip",
MarkdownLinkEip {
pattern: markdown::LinkEip(r"(eip-)([^.]*)\.md(#(.+))?$")
}
),
(
"markdown-link-first",
MarkdownLinkFirst {
Expand All @@ -431,6 +437,12 @@ pub fn default_lints_enum() -> impl Iterator<Item = (&'static str, DefaultLint<&
pattern: markdown::NoBackticks(r"(?i)(eip|erc)-[0-9]+"),
}
),
(
"markdown-link-other",
MarkdownLinkOther {
pattern: markdown::LinkOther(r"(?i)^((?:EIP|ERC)-(\d+)).*$")
}
),
("markdown-rel-links", MarkdownRelativeLinks(markdown::RelativeLinks {
exceptions: vec![
"^https://(www\\.)?github\\.com/ethereum/consensus-specs/blob/[a-f0-9]{40}/.+$",
Expand Down
16 changes: 16 additions & 0 deletions eipw-lint/src/lints/known_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,18 @@ pub enum DefaultLint<S> {

MarkdownHtmlComments(markdown::HtmlComments<S>),
MarkdownJsonSchema(markdown::JsonSchema<S>),
MarkdownLinkEip {
pattern: markdown::LinkEip<S>,
},
MarkdownLinkFirst {
pattern: markdown::LinkFirst<S>,
},
MarkdownNoBackticks {
pattern: markdown::NoBackticks<S>,
},
MarkdownLinkOther {
pattern: markdown::LinkOther<S>,
},
MarkdownLinkStatus(markdown::LinkStatus<S>),
MarkdownProposalRef(markdown::ProposalRef<S>),
MarkdownRegex(markdown::Regex<S>),
Expand Down Expand Up @@ -105,6 +111,8 @@ where

Self::MarkdownHtmlComments(l) => Box::new(l),
Self::MarkdownJsonSchema(l) => Box::new(l),
Self::MarkdownLinkEip { pattern } => Box::new(pattern),
Self::MarkdownLinkOther { pattern } => Box::new(pattern),
Self::MarkdownLinkFirst { pattern } => Box::new(pattern),
Self::MarkdownNoBackticks { pattern } => Box::new(pattern),
Self::MarkdownLinkStatus(l) => Box::new(l),
Expand Down Expand Up @@ -145,8 +153,10 @@ where

Self::MarkdownHtmlComments(l) => l,
Self::MarkdownJsonSchema(l) => l,
Self::MarkdownLinkEip { pattern } => pattern,
Self::MarkdownLinkFirst { pattern } => pattern,
Self::MarkdownNoBackticks { pattern } => pattern,
Self::MarkdownLinkOther { pattern } => pattern,
Self::MarkdownLinkStatus(l) => l,
Self::MarkdownProposalRef(l) => l,
Self::MarkdownRegex(l) => l,
Expand Down Expand Up @@ -262,6 +272,12 @@ where
.map(|(a, b)| (a.as_ref(), b.as_ref()))
.collect(),
}),
Self::MarkdownLinkEip { pattern } => DefaultLint::MarkdownLinkEip {
pattern: markdown::LinkEip(pattern.0.as_ref()),
},
Self::MarkdownLinkOther { pattern } => DefaultLint::MarkdownLinkOther {
pattern: markdown::LinkOther(pattern.0.as_ref()),
},
Self::MarkdownLinkFirst { pattern } => DefaultLint::MarkdownLinkFirst {
pattern: markdown::LinkFirst(pattern.0.as_ref()),
},
Expand Down
4 changes: 4 additions & 0 deletions eipw-lint/src/lints/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
pub mod headings_space;
pub mod html_comments;
pub mod json_schema;
pub mod link_eip;
pub mod link_first;
pub mod link_other;
pub mod link_status;
pub mod no_backticks;
pub mod proposal_ref;
Expand All @@ -19,7 +21,9 @@ pub mod section_required;
pub use self::headings_space::HeadingsSpace;
pub use self::html_comments::HtmlComments;
pub use self::json_schema::JsonSchema;
pub use self::link_eip::LinkEip;
pub use self::link_first::LinkFirst;
pub use self::link_other::LinkOther;
pub use self::link_status::LinkStatus;
pub use self::no_backticks::NoBackticks;
pub use self::proposal_ref::ProposalRef;
Expand Down
177 changes: 177 additions & 0 deletions eipw-lint/src/lints/markdown/link_eip.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use annotate_snippets::snippet::{Annotation, AnnotationType, Slice, Snippet};

use comrak::nodes::{Ast, NodeLink};

use crate::lints::{Context, Error, Lint};
use crate::tree::{self, Next, TraverseExt};

use regex::Regex;

use serde::{Deserialize, Serialize};

use std::fmt::{Debug, Display};

#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct LinkEip<S>(pub S);

impl<S> Lint for LinkEip<S>
where
S: Display + Debug + AsRef<str>,
{
fn lint<'a>(&self, slug: &'a str, ctx: &Context<'a, '_>) -> Result<(), Error> {
let pattern = self.0.as_ref();
let re = Regex::new(pattern).map_err(Error::custom)?;

let mut visitor = Visitor {
ctx,
re,
slug,
link_depth: 0,
text_depth: 0,
current_link: Link {
url: String::new(),
text: String::new(),
},
};
ctx.body().traverse().visit(&mut visitor)?;

Ok(())
}
}
#[derive(Debug)]
struct Link {
url: String,
text: String,
}

#[derive(Debug)]
struct Visitor<'a, 'b, 'c> {
ctx: &'c Context<'a, 'b>,
re: Regex,
slug: &'c str,
link_depth: usize,
text_depth: usize,
current_link: Link,
}

impl<'a, 'b, 'c> Visitor<'a, 'b, 'c> {
fn extract_capture(&self, text: &str, re: &Regex, index: usize) -> Result<String, Error> {
if let Some(captures) = re.captures(text) {
Ok(captures
.get(index)
.map(|m| m.as_str().to_string())
.unwrap_or_default())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that if the regular expression has the wrong number of capture groups, we should inform the user instead of silently returning the empty string.

You can ignore my previous comment about simplifying, and use Error::custom and something like:

#[derive(Debug, Snafu)]
struct BadRegex;

} else {
Ok(String::new())
}
}
Comment on lines +64 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

This function never returns an error, so you can simplify it a bit:

Suggested change
fn extract_capture(&self, text: &str, re: &Regex, index: usize) -> Result<String, Error> {
if let Some(captures) = re.captures(text) {
Ok(captures
.get(index)
.map(|m| m.as_str().to_string())
.unwrap_or_default())
} else {
Ok(String::new())
}
}
fn extract_capture(&self, text: &str, re: &Regex, index: usize) -> String {
if let Some(captures) = re.captures(text) {
captures
.get(index)
.map(|m| m.as_str().to_string())
.unwrap_or_default()
} else {
String::new()
}
}


fn transform_section_description(description: &str) -> String {
let re = Regex::new(r"[-_]").unwrap();
let mut description = re.replace_all(description, " ").to_string();
if let Some(first_char) = description.get_mut(0..1) {
first_char.make_ascii_uppercase();
}
description
}

fn check(&self, ast: &Ast) -> Result<Next, Error> {
let url_eip_text = self.extract_capture(&self.current_link.url, &self.re, 1)?;
let url_eip_number = self.extract_capture(&self.current_link.url, &self.re, 2)?;
let url_section = self.extract_capture(&self.current_link.url, &self.re, 4)?;
Comment on lines +85 to +87
Copy link
Contributor

Choose a reason for hiding this comment

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

This repeats the regex search each time, which is pretty inefficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

eipw-lint already depends on url for parsing URLs. You could use it here to get the fragment (#...).


let dynamic_pattern = if url_section != "" {
format!(r"^(EIP|ERC)-{}(\s*\S+)", regex::escape(&url_eip_number))
} else {
format!(r"^(EIP|ERC)-{}$", regex::escape(&url_eip_number))
};
let text_re = Regex::new(&dynamic_pattern).map_err(Error::custom)?;

if text_re.is_match(&self.current_link.text) && self.text_depth <= 1 {
return Ok(Next::TraverseChildren);
};

let expected = if url_section != "" {
let section_description = Visitor::transform_section_description(&url_section);
format!(
"[{}{}: {}]({})",
url_eip_text.to_uppercase(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Weirdly enough, ERCs are still stored in files named like eip-1234.md, so you can't use the filename to predict whether you need "EIP-..." or "ERC-..."

The correct way to solve this (reading the linked file) won't work for reasons outside eipw's scope, so... I guess the best we can do is something like:

help: use [EIP-1237](./eip-1237.md) or [ERC-1237](./eip-1237.md) instead

url_eip_number,
section_description,
&self.current_link.url
)
} else {
format!(
"[{}{}]({})",
url_eip_text.to_uppercase(),
url_eip_number,
&self.current_link.url
)
};

let footer_label = format!("use `{}` instead", expected);

let source = self
.ctx
.source_for_text(ast.sourcepos.start.line, &self.current_link.text);
self.ctx.report(Snippet {
title: Some(Annotation {
annotation_type: self.ctx.annotation_type(),
id: Some(self.slug),
label: Some("link text does not match link destination"),
}),
slices: vec![Slice {
fold: false,
line_start: ast.sourcepos.start.line,
origin: self.ctx.origin(),
source: &source,
annotations: vec![],
}],
footer: vec![Annotation {
id: None,
annotation_type: AnnotationType::Help,
label: Some(&footer_label),
}],
opt: Default::default(),
})?;

Ok(Next::TraverseChildren)
}
}

impl<'a, 'b, 'c> tree::Visitor for Visitor<'a, 'b, 'c> {
type Error = Error;

fn enter_link(&mut self, _: &Ast, link: &NodeLink) -> Result<Next, Self::Error> {
if self.re.is_match(&link.url) {
self.current_link = Link {
url: link.url.to_owned(),
text: String::new(),
};
self.link_depth += 1;
}
Ok(Next::TraverseChildren)
}

fn depart_link(&mut self, ast: &Ast, _: &NodeLink) -> Result<(), Self::Error> {
if self.link_depth > 0 {
self.check(ast)?;
self.link_depth -= 1;
}
Ok(())
}

fn enter_text(&mut self, ast: &Ast, txt: &str) -> Result<Next, Self::Error> {

Check warning on line 170 in eipw-lint/src/lints/markdown/link_eip.rs

View workflow job for this annotation

GitHub Actions / Test Suite (ubuntu-latest)

unused variable: `ast`

Check warning on line 170 in eipw-lint/src/lints/markdown/link_eip.rs

View workflow job for this annotation

GitHub Actions / Check

unused variable: `ast`
if self.link_depth > 0 {
self.text_depth += 1;
self.current_link.text.push_str(txt);
}
Ok(Next::SkipChildren)
}
}
Loading
Loading