Skip to content

Trim trailing end slashes (and fix formatting)#164

Merged
DZakh merged 4 commits into
mainfrom
url-with-slash
Sep 2, 2024
Merged

Trim trailing end slashes (and fix formatting)#164
DZakh merged 4 commits into
mainfrom
url-with-slash

Conversation

@JasoonS

@JasoonS JasoonS commented Aug 30, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@JasoonS JasoonS requested a review from DZakh August 30, 2024 16:54
Comment on lines +662 to +664
// Trim any trailing slashes from the URL
let trimmed_url = url.trim_end_matches('/').to_string();
Ok(Self::HypersyncConfig(HypersyncConfig { endpoint_url: trimmed_url, is_client_decoder }))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These 3 lines are the only changes, Everything else is just formatting (indentation was wrong)

}

#[test]
fn test_hypersync_url_trailing_slash_trimming() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Decided to add a small test for good measure

@DZakh DZakh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nitpick: In this case I'd replace the validate_url function with the parse_url one, which would either return None, or Some with trimmed URL. Also, looks like we'd like to have the same logic for rpc URLs, not only hypersync

@DZakh DZakh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since you're away, I've done the change myself. @JonoPrest Could you review, please

@DZakh DZakh requested a review from JonoPrest September 2, 2024 07:56

@JonoPrest JonoPrest left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LFG

@DZakh DZakh enabled auto-merge (squash) September 2, 2024 08:27
@DZakh DZakh merged commit 51ab691 into main Sep 2, 2024
@DZakh DZakh deleted the url-with-slash branch September 2, 2024 08:31
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.

3 participants