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

(#150) Replace base URL regex with url-parse #181

Closed
wants to merge 1 commit into from
Closed

Conversation

tlvince
Copy link
Member

@tlvince tlvince commented Nov 15, 2017

Reimplements #160.

Closes #150.
Closes #160.

@tlvince tlvince requested a review from ptitjes November 15, 2017 11:01
Copy link
Collaborator

@ptitjes ptitjes left a comment

Choose a reason for hiding this comment

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

LGTM.

Would you mind adding unit tests for those along the lines "Create a remote DB with a url and check that getUsersUrl(db) is correct (two cases: with/without trailing slash), create a local DB with a simple name and check that getUsersUrl(db) is correct" ?

Also, I'm starting using angular-style commit style to use standard-version for release versioning and changelog generation. That would need to rename your commit "fix(urls): replace base URL regex with url-parse". (The "Closes ..." will be used as well by the changelog generator.)

@ptitjes
Copy link
Collaborator

ptitjes commented Nov 15, 2017

@tlvince BTW, thanks for those. I will close #160.

@ptitjes
Copy link
Collaborator

ptitjes commented Nov 17, 2017

@tlvince Also can you rebase please ? If you don't have time to rebase and add tests, tell me and I will do it.

@ptitjes
Copy link
Collaborator

ptitjes commented Nov 17, 2017

Merged in 7472135.

@ptitjes ptitjes closed this Nov 17, 2017
@ptitjes
Copy link
Collaborator

ptitjes commented Nov 17, 2017

Thank you.

@ptitjes ptitjes deleted the parse-url branch November 23, 2017 17:11
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.

None yet

2 participants