Skip to content

fix: export url website regex#475

Merged
MonilBhavsar merged 5 commits intoExpensify:mainfrom
tienifr:fix/export-url-website-regex
Nov 22, 2022
Merged

fix: export url website regex#475
MonilBhavsar merged 5 commits intoExpensify:mainfrom
tienifr:fix/export-url-website-regex

Conversation

@tienifr
Copy link
Copy Markdown
Contributor

@tienifr tienifr commented Nov 22, 2022

@parasharrajat @MonilBhavsar
I'll export URL_WEBSITE_REGEX from ExpensiMark

Fixed Issues

$ Expensify/App#11810

Tests

QA

@tienifr tienifr requested a review from a team as a code owner November 22, 2022 09:53
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 22, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from luacmartins and removed request for a team November 22, 2022 09:53
@parasharrajat
Copy link
Copy Markdown
Member

@MonilBhavsar Are you ok with the exports here or should we move this to a new file?

@MonilBhavsar
Copy link
Copy Markdown
Contributor

Let's make a new file URL's and have everything related to URLs in it

@MonilBhavsar MonilBhavsar self-requested a review November 22, 2022 09:58
@tienifr
Copy link
Copy Markdown
Contributor Author

tienifr commented Nov 22, 2022

I have read the CLA Document and I hereby sign the CLA

@tienifr
Copy link
Copy Markdown
Contributor Author

tienifr commented Nov 22, 2022

@parasharrajat @MonilBhavsar I've updated my PR, pls let me know your thought. Thanks

return replacedText;
}
}
} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's this change?
Same for another file

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.

oh my bad, I fixed

@MonilBhavsar
Copy link
Copy Markdown
Contributor

@parasharrajat please review

const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_]')}+)?`;
const URL_FRAGMENT_REGEX = `(?:#${addEscapedChar('[-\\w$@.+!*()[\\],=%;\\/:~]')}*)?`;
const URL_REGEX = `(${URL_WEBSITE_REGEX}${URL_PATH_REGEX}(?:${URL_PARAM_REGEX}|${URL_FRAGMENT_REGEX})*)`;
import { URL_REGEX } from './Url';
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.

Suggested change
import { URL_REGEX } from './Url';
import {URL_REGEX} from './Url';

lib/Url.js Outdated
URL_PARAM_REGEX,
URL_FRAGMENT_REGEX,
URL_REGEX
} No newline at end of file
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.

Add a newline at the end of the file.

@tienifr
Copy link
Copy Markdown
Contributor Author

tienifr commented Nov 22, 2022

@parasharrajat I've fixed my PR. Thanks for your review

parasharrajat
parasharrajat previously approved these changes Nov 22, 2022
Copy link
Copy Markdown
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM to me.

cc: @MonilBhavsar Please trigger tests.

@MonilBhavsar
Copy link
Copy Markdown
Contributor

@tienifr one lint error to resolve

@tienifr
Copy link
Copy Markdown
Contributor Author

tienifr commented Nov 22, 2022

@MonilBhavsar Can you help to trigger tests again? Thanks

@MonilBhavsar MonilBhavsar merged commit 87d6599 into Expensify:main Nov 22, 2022
@MonilBhavsar
Copy link
Copy Markdown
Contributor

@tienifr this change will be deployed when this PR is merged Expensify/App#12925
You'll be able to use it in the App after merging main and running npm install.
Meanwhile you can run npm link and test locally https://github.com/Expensify/expensify-common#testing-your-code-while-you-are-developing

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