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

apply skip_prefixes before parsing external link domain #1833

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

mwcz
Copy link
Contributor

@mwcz mwcz commented Apr 25, 2022

From Handling improperly formed external links on the forum.

This PR applies skip_prefixes before attempting to parse the URL.

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

  • Are you doing the PR on the next branch?

Comment on lines +116 to +119
struct LinkDef {
file_path: PathBuf,
external_link: String,
domain: String,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created this struct because I find named fields easier to follow than indexed ones, but I'm happy to switch back to the tuple if that's preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine

@mwcz mwcz force-pushed the skip_by_prefix_before_parsing_url branch from 9d411c2 to 853ffc2 Compare April 25, 2022 15:48
Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

LGTM, do you want to do the strictness change in the same PR?
Any chance you can have a look at #1827 (comment) as well?

Comment on lines +116 to +119
struct LinkDef {
file_path: PathBuf,
external_link: String,
domain: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine

let domain = get_link_domain(&external_link)?;
all_links.push((s.file.path.clone(), external_link, domain));
all_links.push(LinkDef::new(s.file.path.clone(), external_link, domain));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

those 2 sections should really be refactored but I'll do that in my branch.

Copy link
Contributor Author

@mwcz mwcz Apr 26, 2022

Choose a reason for hiding this comment

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

Yeah, I tried combining the iterators with chain so there would only need to be one loop, but couldn't get it to work.

@mwcz
Copy link
Contributor Author

mwcz commented Apr 26, 2022

LGTM, do you want to do the strictness change in the same PR?

Hmm, I think a separate PR would be good, since strictness is a new feature and this one is kinda-sorta bugfixy.

@Keats Keats merged commit 92e80b5 into getzola:next Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants