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

Replace getRepoURL and getRepoPath with getRepositoryInfo #47

Merged
merged 10 commits into from
Nov 7, 2020

Conversation

fregante
Copy link
Member

@fregante fregante commented Nov 1, 2020

Initially added in Refined GitHub, I think we can bring this over, unify it and deprecate any other function.

The 2 functions aren't mapped exactly to getRepositoryInfo. Here are the differences:

  • getRepoURL did not actually check isRepo, so it would return 'settings' if you were on https://github.com/settings
  • getRepoPath did not use the canonical URL, however this probably isn't much of an issue generally

I need to ensure that this doesn't impact the current usage in RGH

@fregante fregante added the enhancement New feature or request label Nov 1, 2020
Copy link
Member Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

cc @yakov116 for any comments

I'll replace the usage of the deprecated functions both here and in RG before merging

index.ts Outdated
}

if (!isRepo(url)) {
return {};
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to return {} instead of undefined to make it easier to check without having to use ?.

// Before:
if (getRepoPath() === undefined) 

// Now:
if (getRepositoryInfo().path === undefined)

// If it returned `undefined`:
if (getRepositoryInfo()?.path === undefined)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed my mind. Returning Partial<RepositoryInfo> is actually wrong because the type is actually RepositoryInfo | {}.

The difference is that with the first one, even if you typecheck if !repository.name, return, .path will still be string | undefined, even if we know for sure it's a string.

Unfortunately RepositoryInfo | {} is awkward to use because repository.name will cause a type error: name doesn't exist on {}.

In short, since we have ?. in the language, let's make use of it for ease of use, even if it generates slightly longer code.

@yakov116
Copy link
Member

yakov116 commented Nov 1, 2020

I debated this when I did the lint. One thing that held me back was that we sometimes need the repo url in lower case. So I backed off changing it all and just kept getRepoURL since I was not sure what needed it.

@fregante
Copy link
Member Author

fregante commented Nov 1, 2020

Good point. I added a couple of notes to my first post as well.

@fregante
Copy link
Member Author

fregante commented Nov 1, 2020

Pre-published as 4.3.0-0

@fregante
Copy link
Member Author

fregante commented Nov 1, 2020

we sometimes need the repo url in lower case

Do we? I think this was solved by our use of og:url in most cases. I'll try to replace our getRepoURL usage everywhere and we can review them in refined-github/refined-github#3695

The only annoyance is that getRepoURL() becomes getRepositoryInfo()!.url and that it's a little slower because it runs more checks.

@yakov116
Copy link
Member

yakov116 commented Nov 1, 2020

Do we?

https://github.com/sindresorhus/refined-github/blob/5b6b9c033e26da85ce9dc164801411da4d50b446/source/features/forked-to.tsx#L81

If I am not mistaken it was added because of that line. Will look for the PR to confirm.

@fregante
Copy link
Member Author

fregante commented Nov 1, 2020

Yeah, I mean we just need to make sure that all the comparisons are made on the same normalized content. In some cases we could still use getRepositoryInfo()!.url.toLowerCase() (😬), but not all.

@fregante
Copy link
Member Author

fregante commented Nov 1, 2020

Last 2 points:

  • rename url to nameWithOwner to match GitHub’s wording and that it's not an actual url
  • in most cases, this is "slower" than it needs to be since it introduces the normalization and isRepo… but I probably shouldn't worry about this

@fregante fregante marked this pull request as ready for review November 1, 2020 05:03
@fregante fregante merged commit 81d47ed into master Nov 7, 2020
@fregante fregante deleted the getRepositoryInfo branch November 7, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants