fix: able to add repo name with "."(dot) in it issue#1451 #1453

us3r64bit · 2022-06-19T11:37:44Z

What type of PR is this? (check all applicable)

Description

This PR fix #1451

Related Tickets & Documents

Mobile & Desktop Screensh

ots/Recordings

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

mtfoley · 2022-06-19T13:51:00Z

@arth2002 thanks for getting this PR started.

@0-vortex with that failing test I guess if what we really care about are the ones that start with github.com, we could make that a third case. WDYT?

0-vortex · 2022-06-19T14:51:44Z

@arth2002 thanks for getting this PR started.

@0-vortex with that failing test I guess if what we really care about are the ones that start with github.com, we could make that a third case. WDYT?

We should support all repos, this should fix things not become a breaking change. Changing from includes->substr will do the trick - it's a "relative" URL only if it starts with those matchers, not if it's included in repoName

mtfoley · 2022-06-21T03:18:41Z

I checked out this PR locally, and I actually think the utils.js would work well like this:

export function isValidRepoUrl(url) {
  
  return fullnameValidator(urlToFullName(url));
}

function urlToFullName(url){
  const fullname = url.replace(/^(http|https):\/\//i, "") // take off URL protocol
    .replace(/^(www\.)*github\.com\//i, "") // take off github.com variant
    .replace(/^\/*/ ,"")// take off leading slash(es) if there
    .replace(/ /g, ""); // take out spaces
  return fullname;
}

function fullnameValidator(fullname){
  // take only first two elements
  const [owner, repo] = fullname.split("/");
  if(!owner || !repo) return [false, null];
  // alphanumerics and hyphens are allowed in owner names
  const invalidOwnerRegex = /[^a-z0-9-]+/i;
  // alphanumerics, hyphens, underscores, and dots are allowed in repo names
  const invalidRepoRegex = /[^a-z0-9-_\.]+/i;
  const valid = !invalidOwnerRegex.test(owner) && !invalidRepoRegex.test(repo);
  // if invalid, return [false, null]
  // if valid, return [true, fullname]
  return [valid, valid ? fullname : null];
}

@arth2002 I think the code above needs some more tweaking but I wanted to bounce the idea off of you and @0-vortex.

The test file actually has some issues as well so I'll follow up with a later comment.

0-vortex · 2022-06-21T09:32:12Z

I checked out this PR locally, and I actually think the utils.js would work well like this:

export function isValidRepoUrl(url) {
  
  return fullnameValidator(urlToFullName(url));
}

function urlToFullName(url){
  const fullname = url.replace(/^(http|https):\/\//i, "") // take off URL protocol
    .replace(/^(www\.)*github\.com\//i, "") // take off github.com variant
    .replace(/^\/*/ ,"")// take off leading slash(es) if there
    .replace(/ /g, ""); // take out spaces
  return fullname;
}

function fullnameValidator(fullname){
  // take only first two elements
  const [owner, repo] = fullname.split("/");
  if(!owner || !repo) return [false, null];
  // alphanumerics and hyphens are allowed in owner names
  const invalidOwnerRegex = /[^a-z0-9-]+/i;
  // alphanumerics, hyphens, underscores, and dots are allowed in repo names
  const invalidRepoRegex = /[^a-z0-9-_\.]+/i;
  const valid = !invalidOwnerRegex.test(owner) && !invalidRepoRegex.test(repo);
  // if invalid, return [false, null]
  // if valid, return [true, fullname]
  return [valid, valid ? fullname : null];
}

@arth2002 I think the code above needs some more tweaking but I wanted to bounce the idea off of you and @0-vortex.

The test file actually has some issues as well so I'll follow up with a later comment.

Hard no on this - you have taken a function that checks whether a subset of the URL RFC is relative or not and turned it into a URL validator, that's potentially the opposite of what we are trying to do here; the regexp approach sounds good in theory but if you take a look at RFC 1738 you will understand why this approach is so bad. You can spend weeks perfecting regular expressions like this.

To make a better example, I mentioned in the previous review that the third includes should be changed into a substr, that is, essentially, to check for that condition at the beginning of the string. If you use regular expressions that can be simplified into ^(https?:\/\/)*(www\.?)*([a-z]+\.[a-z]+\/)*, checking for all the parts of a URL and excluding them as they appear you get the problem that it has to be a perfect regex, all the parts have to work in all cases, which makes the difficulty of adjusting to all those use cases, terrible. The above regex would cover the following use cases:

brob/plug11ty.com
github.com/brob/plug11ty.com
www.github.com/brob/plug11ty.com
https://github.com/brob/plug11ty.com
https://www.github.com/brob/plug11ty.com

However, that is going full-on into scope creep. The existing code is meant to catch 3 things at the beginning of the string:

http (done with substr)
https (done with substr)
www (done with regex without ^)

Add the ^ to the third check or transform the regex into a substr

mtfoley · 2022-06-21T13:47:51Z

Fair enough. @arth2002 is the instruction clear from what @0-vortex was saying?

us3r64bit · 2022-06-21T15:45:25Z

Fair enough. @arth2002 is the instruction clear from what @0-vortex was saying?

Hey @mtfoley sorry to bother you sir
I just want to check we're on the same page, as per I understood do I need to replace
this code

const isRelativeUrl = !(url.substr(0, 4) === "http" || url.includes(".com") || url.includes("www."));

with this one

 const isRelativeUrl = !(url.substr(0,7) === 'http://' || url.substr(0,8) === 'https://' || url.substr(0,4) === 'www.');

am I correct? if not can please correct me
Thank You :)

0-vortex · 2022-06-22T23:02:06Z

Fair enough. @arth2002 is the instruction clear from what @0-vortex was saying?

Hey @mtfoley sorry to bother you sir I just want to check we're on the same page, as per I understood do I need to replace this code
const isRelativeUrl = !(url.substr(0, 4) === "http" || url.includes(".com") || url.includes("www."));
with this one
 const isRelativeUrl = !(url.substr(0,7) === 'http://' || url.substr(0,8) === 'https://' || url.substr(0,4) === 'www.');
am I correct? if not can please correct me Thank You :)

In case you missed the 👍 emoji, it's good! :D

us3r64bit · 2022-06-23T06:09:27Z

Please review it

mtfoley

@arth2002, I think it would also be helpful to add a couple of tests. The two cases I'd suggest would both be "valid", and would be:

the repo of the original issue posting ("brob/plug11ty.com")
the repo "httpie/httpie", just to verify that use case works also.

src/lib/util.js

mtfoley · 2022-06-24T18:47:50Z

src/lib/util.js

@@ -1,8 +1,8 @@
 export function isValidRepoUrl(url) {
-  url = url.trim();
+  url = url.toLowerCase().trim();
  url = url.substr(0, 1) === "/" ? url.substr(1) : url;


@arth2002 I just want you to know you're being very patient with me! That said, I'd suggest we take a moment to change url.substr() to be url.substring(). It looks like substr() is deprecated (though widely supported). https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr#browser_compatibility

mtfoley · 2022-06-25T19:47:37Z

Looks good to me!

### [0.52.1](v0.52.0...v0.52.1) (2022-06-25) ### 🐛 Bug Fixes * able to add repo name with "."(dot) in it issue[#1451](#1451) ([#1453](#1453)) ([fcca807](fcca807))

github-actions · 2022-06-25T19:50:41Z

🎉 This PR is included in version 0.52.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

us3r64bit added 3 commits June 19, 2022 16:14

issue#1451

2c93a6e

restart

8785d2f

fix issue with repo name

ec19675

updated util.js

03f63d8

mtfoley self-requested a review June 24, 2022 00:38

mtfoley reviewed Jun 24, 2022

View reviewed changes

src/lib/util.js Outdated Show resolved Hide resolved

updated util.js

99f34a9

mtfoley reviewed Jun 24, 2022

View reviewed changes

updated url.substr() to url.substring()

83788e3

mtfoley merged commit fcca807 into open-sauced:main Jun 25, 2022

github-actions bot added the released label Jun 25, 2022

us3r64bit deleted the fix-repo-name-issue branch June 26, 2022 05:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: able to add repo name with "."(dot) in it issue#1451 #1453

fix: able to add repo name with "."(dot) in it issue#1451 #1453

us3r64bit commented Jun 19, 2022 •

edited

mtfoley commented Jun 19, 2022

0-vortex commented Jun 19, 2022

mtfoley commented Jun 21, 2022

0-vortex commented Jun 21, 2022

mtfoley commented Jun 21, 2022

us3r64bit commented Jun 21, 2022

0-vortex commented Jun 22, 2022

us3r64bit commented Jun 23, 2022

mtfoley left a comment

mtfoley Jun 24, 2022

us3r64bit Jun 25, 2022

mtfoley commented Jun 25, 2022

github-actions bot commented Jun 25, 2022

fix: able to add repo name with "."(dot) in it issue#1451 #1453

fix: able to add repo name with "."(dot) in it issue#1451 #1453

Conversation

us3r64bit commented Jun 19, 2022 • edited

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screensh

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

mtfoley commented Jun 19, 2022

0-vortex commented Jun 19, 2022

mtfoley commented Jun 21, 2022

0-vortex commented Jun 21, 2022

mtfoley commented Jun 21, 2022

us3r64bit commented Jun 21, 2022

0-vortex commented Jun 22, 2022

us3r64bit commented Jun 23, 2022

mtfoley left a comment

Choose a reason for hiding this comment

mtfoley Jun 24, 2022

Choose a reason for hiding this comment

us3r64bit Jun 25, 2022

Choose a reason for hiding this comment

mtfoley commented Jun 25, 2022

github-actions bot commented Jun 25, 2022

us3r64bit commented Jun 19, 2022 •

edited