Lints to ensure link text for EIPs should match the EIP's number by KatyaRyazantseva · Pull Request #99 · ethereum/eipw

KatyaRyazantseva · 2024-06-25T19:10:32Z

Issue 67. Covered examples with tests.

KatyaRyazantseva · 2024-07-03T10:22:56Z

SamWilsn

Overall, I'd try to avoid hard coding regexes where possible. If they're hard coded, they can't be changed by the configuration file.

SamWilsn · 2024-07-03T14:13:19Z

eipw-lint/src/lib.rs

+        (
+            "markdown-link-other",
+            MarkdownLinkOther {
+                pattern: markdown::LinkOther(r"^(EIP|ERC)-(\d+)\s*\S*$")


Should this be case insensitive ((?i))?

I am not sure about it. EIP-1 says the text should be like EIP-N. Does it mean upper case or not? I can add case insensitive if it is so. Then eip-25 will work for links. Is it ok or it must be EIP-25 (upper case only)?

I think we have the other lint to check for incorrectly cased references. So even if [eip-1](./eip-1.md) passes markdown-link-other, it'll fail the other lint.

Or at least I think it will 🤔

SamWilsn · 2024-07-03T14:14:55Z

eipw-lint/src/lints/markdown/link_eip.rs

+        if self.link_depth > 0 {
+            self.link_depth = self.link_depth.checked_sub(1).unwrap();
+        }


If self.link_depth is greater than zero, the checked_sub is unnecessary.

checked_sub in the depart_link function returns self.link_depth to 0. I check link_depth in enter_text fn. If link_depth == 0, it means the text is not from the link, so, I skip it.

What I mean is that if self.link_depth > 0 then self.link_depth - 1 can never underflow. The checked_sub will never fail.

SamWilsn · 2024-07-03T14:17:25Z

eipw-lint/src/lints/markdown/link_eip.rs

+        if self.link_depth > 0 {
+            self.current_link.text = txt.to_owned();
+            self.check(ast)?;         
+        }


How does this behave for content like:

[**EIP-1**5678](./eip-1.md#rationale)

It fails. I will add it to the tests. Should it fail?

Hm, perhaps I spoke too soon then 🤣

I was worried that because every text node in the link sets self.current_link.text, you could end up in situations where, for example, bold would create two text nodes, breaking the lint. Maybe something like [**EIP-1**EIP-1](./eip-1.md).

If my concerns are unfounded, ignore me!

SamWilsn · 2024-07-03T14:20:19Z

eipw-lint/tests/lint_markdown_link_eip.rs

+  |
+4 | [EIP-1](./eip-2.md)
+  |
+  = info: link text should match `[EIP|ERC-2]`


If you can, I'd rather the specific expected text in the message. Maybe something like:

error[markdown-link-eip]: link text does not match link destination | 4 | [EIP-1](./eip-2.md) | = help: `use [EIP-2](./eip-2.md)` instead

I am working on the help hint. I check the link and build the correct version of the text based on the link. Is it correct? So, my hints will look like this:

[EIP-1](./eip-2.md) -> [EIP-2](./eip-2.md)
[EIP-1: Foo](./eip-1.md) -> [EIP-1](./eip-1.md)
[Another Proposal](./eip-1.md) -> [EIP-1](./eip-1.md)
[EIP-1](./eip-1.md#motivation) -> [EIP-1: Motivation](./eip-1.md#motivation)
[EIP-2: Hello](./eip-1.md#abstract) -> [EIP-1: Abstract](./eip-1.md#abstract)
[Another Proposal](./eip-1.md#rationale) -> [EIP-1: Rationale](./eip-1.md#rationale)

Example:

error[markdown-link-eip]: link text does not match link destination | 4 | [Another Proposal](./eip-1.md#rationale) | = help: ` [EIP-1: Rationale](./eip-1.md#rationale)` instead

That looks great!

SamWilsn · 2024-07-03T14:21:20Z

eipw-lint/tests/lint_markdown_link_eip.rs

+  |
+4 | [Another Proposal](./eip-1.md#rationale)
+  |
+  = info: link text should match `[EIP|ERC-1<section-description>]`


Similar kind of comment here. The help text should (if possible) include the correct text to use. Authors might not understand how to make something "match".

KatyaRyazantseva · 2024-07-05T14:39:35Z

Pushed updates. I tried to fix everything according to comments. Haven't found EIP-1 example in existing eips. So, for now ignoring you)) Can we create a new issue for it in case it comes later?

Hardcoded regexes in lib.rs are kind of a style for the whole file, I followed the pattern. If we need to change regexes in config files, we need to refactor it globally.

SamWilsn · 2024-07-06T15:09:24Z

Hardcoded regexes in lib.rs are kind of a style for the whole file, I followed the pattern. If we need to change regexes in config files, we need to refactor it globally.

The configuration in lib.rs (default_lints_enum is just the defaults. Hard coding there is fine because anything there can be overridden by the config file.

Other regexes, like this one cannot be overridden.

SamWilsn · 2024-07-06T15:33:46Z

I've added a test in 09d7de7 that demonstrates the problem I brought up in #99 (comment).

KatyaRyazantseva · 2024-07-06T17:22:28Z

Hardcoded regexes in lib.rs are kind of a style for the whole file, I followed the pattern. If we need to change regexes in config files, we need to refactor it globally.

The configuration in lib.rs (default_lints_enum is just the defaults. Hard coding there is fine because anything there can be overridden by the config file.

Other regexes, like this one cannot be overridden.

This one is dynamic. It depends on the lib.rs pattern. I extract the number of eip there and put into a new regex. Do you have any better ideas?

KatyaRyazantseva · 2024-07-06T18:13:37Z

fixed bold text

KatyaRyazantseva · 2024-07-26T12:14:14Z

@SamWilsn, I've done with the fixes. Could you please review it and merge if there are no any comments?

SamWilsn · 2024-08-23T19:03:21Z

eipw-lint/src/lints/markdown/link_eip.rs

+    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())
+        }
+    }


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()

}

}

SamWilsn · 2024-08-23T19:12:44Z

eipw-lint/src/lints/markdown/link_eip.rs

+            Ok(captures
+                .get(index)
+                .map(|m| m.as_str().to_string())
+                .unwrap_or_default())


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;

SamWilsn · 2024-08-23T19:15:40Z

eipw-lint/src/lints/markdown/link_eip.rs

+        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)?;


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

SamWilsn · 2024-08-23T19:22:04Z

eipw-lint/src/lints/markdown/link_eip.rs

+    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)?;


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

SamWilsn · 2024-08-23T19:38:47Z

eipw-lint/src/lints/markdown/link_eip.rs

+            let section_description = Visitor::transform_section_description(&url_section);
+            format!(
+                "[{}{}: {}]({})",
+                url_eip_text.to_uppercase(),


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

…nto issue-67

KatyaRyazantseva · 2024-08-24T21:34:12Z

@SamWilsn I updated Rust (1.80.1 the latest stable version) and now have this issue with time crates rust-lang/rust#127343. Can't build the project anymore. Can we somehow fix it? Delete it from Cargo.lock? I haven't found it elsewhere in the project.

SamWilsn · 2024-09-04T14:40:44Z

If you're using rustup, you can use, for example, cargo +1.69 build to pick a particular version.

I'll update the rust version in a separate PR.

KatyaRyazantseva added 2 commits May 30, 2024 16:58

initial markdown-link-eip

fd7cebb

update markdown-link-eip, add markdown-link-other

011babb

SamWilsn reviewed Jul 3, 2024

View reviewed changes

bug fixes

2829f03

SamWilsn added 2 commits July 6, 2024 11:31

Format and fix tests

4930ed4

Add test for repeated text in markdown-link-eip

09d7de7

Add second test

0df9027

add bold text check

cdfc4d0

KatyaRyazantseva mentioned this pull request Jul 26, 2024

Weekly Call [Cohort 1] wiepteam/studygroup#1

Closed

2 tasks

poojaranjan mentioned this pull request Aug 2, 2024

Weekly Call [Cohort 1] wiepteam/studygroup#4

Closed

3 tasks

initial markdown-link-eip

8772403

SamWilsn force-pushed the issue-67 branch from cdfc4d0 to 82704cb Compare August 23, 2024 19:02

KatyaRyazantseva and others added 6 commits August 23, 2024 15:17

update markdown-link-eip, add markdown-link-other

04b0e07

bug fixes

47a8bbd

Format and fix tests

ad8e022

Add test for repeated text in markdown-link-eip

0da655d

Add second test

680beea

add bold text check

1cbd715

SamWilsn force-pushed the issue-67 branch from 82704cb to 1cbd715 Compare August 23, 2024 19:18

SamWilsn reviewed Aug 23, 2024

View reviewed changes

Merge branch 'issue-67' of https://github.com/KatyaRyazantseva/eipw i…

eef80dd

…nto issue-67

Merge origin/master into issue-67

550e0d2

Conversation

KatyaRyazantseva commented Jun 25, 2024

Uh oh!

KatyaRyazantseva commented Jul 3, 2024

Uh oh!

SamWilsn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KatyaRyazantseva commented Jul 5, 2024

Uh oh!

SamWilsn commented Jul 6, 2024

Uh oh!

SamWilsn commented Jul 6, 2024

Uh oh!

KatyaRyazantseva commented Jul 6, 2024

Uh oh!

KatyaRyazantseva commented Jul 6, 2024

Uh oh!

KatyaRyazantseva commented Jul 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KatyaRyazantseva commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamWilsn commented Sep 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KatyaRyazantseva commented Aug 24, 2024 •

edited

Loading