-
Notifications
You must be signed in to change notification settings - Fork 4
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
anujchoudhary-17 : Fixed - Error loading local files on Windows machines #28 issue #44
base: main
Are you sure you want to change the base?
Conversation
… verify with regex
@@ -78,6 +83,16 @@ const getData = async (location: string): Promise<string> => { | |||
throw new Error(`Could not determine if ${location} is a URL or file`); | |||
}; | |||
|
|||
|
|||
const validatingURLusingRegex = (locationString : string): Boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only checking if the protocol is valid, not the rest of the URL, I would recommend either:
- Renaming the function to something like
isHttpOrHttps(locationString: string): boolean
- Validating the rest of the URL with something like the following (untested)
const validateUrl = (location: string): boolean => {
try {
const url = new URL(location);
return url.protocol === "http:" || url.protocol === "https:";
} catch {
return false;
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, Thank you @mrjones2014 :) for the feedback. I am new to Open Source so just trying my best to contribute. I will update it and raise the PR again :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love to see new open source contributors ❤️
Please feel free to let me know if you need any help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mrjones2014 ,
Updated the branch and implemented the second option you suggested. Please review it :)
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @anujchoudhary-17, thanks for opening this, and we're so excited for your contributions to 1Password's open source projects! Couple things I wanted to note:
- All our repositories require commits to signed before we can merge to the main branch. I know from your other PR that you've just got set up with commit signing, so let me know if you run into any troubles.
- For this to merge we need to maintain a consistent code format - while you can set up your IDE to do this for you automatically on save, you can also run
yarn format:write
from the root of this project to auto-format all your code. - Similarly, it appears there are a handful of linting errors that are failing CI. Please take a moment to review them (you can run
yarn lint:scripts
to check this out locally) and submit your changes.
Thanks again for taking the time to contribute. Please let me know if you need a hand with anything 🙂
Hi @jodyheavener , but when I try to run yarn lint:scripts Not sure how to solve it. |
Hi @anujchoudhary-17 those are some interesting results you're seeing. I have not previously run this project on a Windows machine, but I don't think that should matter. Please ensure that you are using Node 16 and have run |
@jodyheavener Yes I have checked all those things already it is exactly what you are expecting. Still getting the same issue :( |
Please review src\index.ts added a boolean function name validatingURLusingRegex to verify with regex which checks whether the start of the url string is either http or https.
Issue link : #28