Add lint to prevent (most) URLs #104 #106

KoxyG · 2024-09-04T22:55:45Z

Pls review and let me know if there is anything that needs changes @SamWilsn

SamWilsn · 2024-09-05T13:33:45Z

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

+         // Report the issue
+         let source = self.ctx.source_for_text(ast.sourcepos.start.line, text);
+         let offending_url = self.re.find(text).unwrap().as_str();
+         let suggestion_label = format!("Avoid using backticks in URLs: `{}`", offending_url);


Haven't investigated the code yet, but this text seems to indicate it does the opposite of what we need.

The lint is supposed to make sure authors don't put URLs in backticks, not that they don't put backticks in URLs.

SamWilsn · 2024-09-05T13:39:14Z

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

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


This should probably look something like:

Suggested change

#[derive(Debug, Serialize, Deserialize, Clone)]

pub struct PreventUrlsNoBackticks<S>(pub S);

#[derive(Debug, Serialize, Deserialize, Clone)]

pub struct PreventUrlsNoBackticks<S> {

allowed_domains: Vec<S>,

}

Then the default configuration in lib.rs would look like:

( "markdown-prevent-url", MarkdownPreventUrl { allowed_domains: vec![ r"(.+\.)?example.com", r"(.+\.)?invalid", // And so on... ] } ),

SamWilsn · 2024-09-05T13:42:12Z

eipw-lint/tests/lint_markdown_prevent_url.rs

+ header: value1
+ ---
+
+ Check this URL: `http://example.com/page?query=foo`


You should write tests based on the examples in #104. This URL is very close to one of the examples I gave that should not trigger the lint.

In other words, this URL should be allowed because its domain is example.com—one of the allowed example domains at the top of #104.

KoxyG · 2024-09-07T19:08:19Z

Hi @SamWilsn I just made changes. Kindly check and help review.

SamWilsn

Quick review. You'll need to resolve the compilation issues before we can go any further.

SamWilsn · 2024-09-30T19:53:37Z

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

+
+         // Regex to match URLs not from allowed domains and containing backtick
+
+        let re = Regex::new(r"https?://(?:example\.com|example\.net|example\.org|example|invalid|test)(?:/[^`\s]*)?").unwrap();


I haven't quite figured out what this regex is for yet, but you shouldn't hardcode the list of domains here. It could get out of sync with self.allowed_domains.

SamWilsn · 2024-09-30T19:57:54Z

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

+             // If the URL is in backticks, report it
+             if in_backticks {
+                let source = self.ctx.source_for_text(ast.sourcepos.start.line, text);
+                let message = format!("URLs are not allowed in backticks: `{}`", url);


Suggested change

let message = format!("URLs are not allowed in backticks: `{}`", url);

let message = "Valid URLs are not allowed in backticks";

I think we can use a simpler error message here.

SamWilsn · 2024-09-30T19:59:03Z

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

+                    title: Some(Annotation {
+                        annotation_type: self.ctx.annotation_type(),
+                        id: Some(self.slug),
+                        label: Some("not from allowed domain found"),


Suggested change

label: Some("not from allowed domain found"),

label: Some("non-reserved domain"),

How about that?

SamWilsn · 2024-09-30T20:00:39Z

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

+            // Report other disallowed URLs
+            let source = self.ctx.source_for_text(ast.sourcepos.start.line, text);
+            let suggestion_label = format!("This URL must be hyperlinked or from an allowed domain: `{}`", url);
+
+            self.ctx.report(Snippet {
+                opt: Default::default(),
+                title: Some(Annotation {
+                    annotation_type: self.ctx.annotation_type(),
+                    id: Some(self.slug),
+                    label: Some("Disallowed URL in plain text or code fence"),
+                }),
+                slices: vec![Slice {
+                    fold: false,
+                    line_start: ast.sourcepos.start.line,
+                    origin: self.ctx.origin(),
+                    source: &source,
+                    annotations: vec![],
+                }],
+                footer: vec![Annotation {
+                    annotation_type: AnnotationType::Help,
+                    id: None,
+                    label: Some(&suggestion_label),
+                }],
+            })?;


It's not immediately obvious why this section is separate from the above one. Could you explain?

SamWilsn · 2024-09-30T20:01:17Z

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

+ impl<'a, 'b, 'c> tree::Visitor for Visitor<'a, 'b, 'c> {
+     type Error = Error;
+
+     fn enter_link(&mut self, _ast: &Ast, _link: &str) -> Result<Next, Self::Error> {
+            Ok(Next::TraverseChildren)
+     }
+
+     fn enter_code(&mut self, ast: &Ast, code: &comrak::nodes::NodeCode) -> Result<Next, Self::Error> {
+         self.check(ast, &code.literal, false, false)
+     }
+
+     fn enter_text(&mut self, ast: &Ast, text: &str) -> Result<Next, Self::Error> {
+         self.check(ast, text, false, false)
+     }
+
+     fn enter_html_inline(&mut self, ast: &Ast, html: &str) -> Result<Next, Self::Error> {
+        // For inline HTML links like <https://link>
+        self.check(ast, html, true, false)
+    }
+
+    fn enter_code_block(&mut self, ast: &Ast, block: &comrak::nodes::NodeCodeBlock) -> Result<Next, Self::Error> {
+        let code_text = String::from_utf8_lossy(&block.literal);
+        self.check(ast, &code_text, false, false)
+    }
+
+
+ } 


You're missing a few cases here I think.

KoxyG added 2 commits September 4, 2024 23:47

worked on prevent url with backticks or that are not hyperlinked

7c0c704

Add lint to prevent (most) URLs ethereum#104

7cf8a4f

KoxyG mentioned this pull request Sep 5, 2024

Add lint to prevent (most) URLs #104

Open

SamWilsn requested changes Sep 5, 2024

View reviewed changes

KoxyG added 2 commits September 7, 2024 20:00

avoid url in backticks

71e2cc1

url in backticks

8a0af04

SamWilsn added 5 commits September 30, 2024 15:46

Update README.md

187d6e7

Update relative_links.rs

a93db36

Update README.md

a206b44

Update README.md

8a4d978

Update headings_space.rs

f78a08e

SamWilsn reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint to prevent (most) URLs #104 #106

Add lint to prevent (most) URLs #104 #106

KoxyG commented Sep 4, 2024

SamWilsn Sep 5, 2024

SamWilsn Sep 5, 2024

SamWilsn Sep 5, 2024

KoxyG commented Sep 7, 2024 •

edited

Loading

SamWilsn left a comment

SamWilsn Sep 30, 2024

SamWilsn Sep 30, 2024

SamWilsn Sep 30, 2024

SamWilsn Sep 30, 2024

SamWilsn Sep 30, 2024

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


		// Regex to match URLs not from allowed domains and containing backtick

		let re = Regex::new(r"https?://(?:example\.com\|example\.net\|example\.org\|example\|invalid\|test)(?:/[^`\s]*)?").unwrap();

	let message = format!("URLs are not allowed in backticks: `{}`", url);
	let message = "Valid URLs are not allowed in backticks";

	label: Some("not from allowed domain found"),
	label: Some("non-reserved domain"),

Add lint to prevent (most) URLs #104 #106

Are you sure you want to change the base?

Add lint to prevent (most) URLs #104 #106

Conversation

KoxyG commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoxyG commented Sep 7, 2024 • edited Loading

SamWilsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoxyG commented Sep 7, 2024 •

edited

Loading