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 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Format and fix tests
  • Loading branch information
SamWilsn committed Jul 6, 2024
commit 4930ed4a8ac5b93823eb0ad294eba8089ed9767b
6 changes: 3 additions & 3 deletions eipw-lint/src/lints/known_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ pub enum DefaultLint<S> {
MarkdownHtmlComments(markdown::HtmlComments<S>),
MarkdownJsonSchema(markdown::JsonSchema<S>),
MarkdownLinkEip {
pattern: markdown::LinkEip<S>
pattern: markdown::LinkEip<S>,
},
MarkdownLinkFirst {
pattern: markdown::LinkFirst<S>,
},
MarkdownLinkOther {
pattern: markdown::LinkOther<S>
pattern: markdown::LinkOther<S>,
},
MarkdownLinkStatus(markdown::LinkStatus<S>),
MarkdownProposalRef(markdown::ProposalRef<S>),
Expand Down Expand Up @@ -291,7 +291,7 @@ where
prefix: l.prefix.as_ref(),
suffix: l.suffix.as_ref(),
})
},
}
Self::MarkdownRegex(l) => DefaultLint::MarkdownRegex(markdown::Regex {
message: l.message.as_ref(),
mode: l.mode,
Expand Down
72 changes: 47 additions & 25 deletions eipw-lint/src/lints/markdown/link_eip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,19 @@
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>,
Expand All @@ -33,7 +33,10 @@ where
re,
slug,
link_depth: 0,
current_link: Link { url: String::new(), text: String::new() },
current_link: Link {
url: String::new(),
text: String::new(),
},
};
ctx.body().traverse().visit(&mut visitor)?;

Expand All @@ -58,7 +61,10 @@ struct Visitor<'a, 'b, 'c> {
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())
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())
}
Expand All @@ -73,32 +79,45 @@ impl<'a, 'b, 'c> Visitor<'a, 'b, 'c> {
description
}

fn check(&self, ast: &Ast) -> Result<Next, Error> {
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)?;
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 != "" {

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) {
return Ok(Next::TraverseChildren);
};

let expected = if url_section != "" {
let section_description = Visitor::transform_section_description(&url_section);
format!("[{}{}: {}]({})", url_eip_text.to_uppercase(), url_eip_number, section_description, &self.current_link.url)
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)
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);
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(),
Expand All @@ -123,15 +142,18 @@ impl<'a, 'b, 'c> Visitor<'a, 'b, 'c> {
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;
}
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)
}

Expand All @@ -145,8 +167,8 @@ impl<'a, 'b, 'c> tree::Visitor for Visitor<'a, 'b, 'c> {
fn enter_text(&mut self, ast: &Ast, txt: &str) -> Result<Next, Self::Error> {
if self.link_depth > 0 {
self.current_link.text = txt.to_owned();
self.check(ast)?;
self.check(ast)?;
}
Ok(Next::SkipChildren)
Ok(Next::SkipChildren)
}
}
}
63 changes: 41 additions & 22 deletions eipw-lint/src/lints/markdown/link_other.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,19 @@
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 LinkOther<S>(pub S);

impl<S> Lint for LinkOther<S>
where
S: Display + Debug + AsRef<str>,
Expand All @@ -33,7 +33,10 @@ where
re,
slug,
link_depth: 0,
current_link: Link { url: String::new(), text: String::new() },
current_link: Link {
url: String::new(),
text: String::new(),
},
};
ctx.body().traverse().visit(&mut visitor)?;

Expand All @@ -58,27 +61,40 @@ struct Visitor<'a, 'b, 'c> {
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())
Ok(captures
.get(index)
.map(|m| m.as_str().to_string())
.unwrap_or_default())
} else {
Ok(String::new())
}
}

fn check(&self, ast: &Ast) -> Result<Next, Error> {
let text_eip_full = self.extract_capture(&self.current_link.text, &self.re, 1)?;
let text_eip_number = self.extract_capture(&self.current_link.text, &self.re, 2)?;
fn check(&self, ast: &Ast) -> Result<Next, Error> {
let text_eip_full = self.extract_capture(&self.current_link.text, &self.re, 1)?;
let text_eip_number = self.extract_capture(&self.current_link.text, &self.re, 2)?;

let dynamic_pattern = format!(r"(?i)\beip-{}\b", regex::escape(&text_eip_number));
let url_re = Regex::new(&dynamic_pattern).map_err(Error::custom)?;

if url_re.is_match(&self.current_link.url) {
return Ok(Next::TraverseChildren);
}

let expected = format!("[{}](./{}.md)", text_eip_full.to_uppercase(), text_eip_full.to_lowercase());
let footer_label = format!("the link destination should target {}, for example `{}`", text_eip_full.to_uppercase(), expected);

let source = self.ctx.source_for_text(ast.sourcepos.start.line, &self.current_link.text);
let expected = format!(
"[{}](./{}.md)",
text_eip_full.to_uppercase(),
text_eip_full.to_lowercase()
);
let footer_label = format!(
"the link destination should target {}, for example `{}`",
text_eip_full.to_uppercase(),
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(),
Expand Down Expand Up @@ -107,9 +123,12 @@ impl<'a, 'b, 'c> Visitor<'a, 'b, 'c> {
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> {
self.current_link = Link { url: link.url.to_owned(), text: String::new() };
self.link_depth += 1;
fn enter_link(&mut self, _: &Ast, link: &NodeLink) -> Result<Next, Self::Error> {
self.current_link = Link {
url: link.url.to_owned(),
text: String::new(),
};
self.link_depth += 1;
Ok(Next::TraverseChildren)
}

Expand All @@ -121,10 +140,10 @@ impl<'a, 'b, 'c> tree::Visitor for Visitor<'a, 'b, 'c> {
}

fn enter_text(&mut self, ast: &Ast, txt: &str) -> Result<Next, Self::Error> {
if self.link_depth > 0 && self.re.is_match(&txt) {
if self.link_depth > 0 && self.re.is_match(&txt) {
self.current_link.text = txt.to_owned();
self.check(ast)?;
self.check(ast)?;
}
Ok(Next::TraverseChildren)
Ok(Next::TraverseChildren)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ error[markdown-link-status]: proposal `eip-20.md` is not stable enough for a `st
error[markdown-link-status]: proposal `eip-2048.md` is not stable enough for a `status` of `Last Call`
--> input.md
|
21 | This is the specification for the EIP, [for some reason](./eip-2048.md).
21 | This is the specification for the EIP, [EIP-2048](./eip-2048.md).
|
= help: because of this link, this proposal's `status` must be one of: `Draft`, `Stagnant`
2 changes: 1 addition & 1 deletion eipw-lint/tests/eipv/markdown-link-too-unstable/input.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ This is the abstract for the EIP which needs [EIP-20](./eip-20.md).
This is the motivation for the EIP, which links to [EIP-1337](./eip-1337.md).

## Specification
This is the specification for the EIP, [for some reason](./eip-2048.md).
This is the specification for the EIP, [EIP-2048](./eip-2048.md).

## Rationale
This is the rationale for the EIP.
Expand Down
2 changes: 1 addition & 1 deletion eipw-lint/tests/eipv/markdown-unlinked-eip/input.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ This is also the abstract for the EIP, which extends ERC-1236.
## Motivation
This is the motivation for the EIP, which is separate from [EIP-1235](./eip-1235.md).

This is also the abstract for the EIP, which extends [ERC-1237](./eip-1236.md).
This is also the abstract for the EIP, which extends [ERC-1237](./eip-1237.md).

## Specification
This is the specification for the EIP, mentioning EIP-1235 and ERC-1237 again.
Expand Down
Loading