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

Ask user to select PR templates when forking a repository #143733

Merged
merged 17 commits into from Apr 1, 2022

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Feb 23, 2022

This PR fixes #143083

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
extensions/github/src/pushErrorHandler.ts Outdated Show resolved Hide resolved
extensions/github/src/pushErrorHandler.ts Outdated Show resolved Hide resolved
extensions/github/src/pushErrorHandler.ts Show resolved Hide resolved
extensions/github/src/pushErrorHandler.ts Outdated Show resolved Hide resolved
extensions/github/src/pushErrorHandler.ts Outdated Show resolved Hide resolved
babakks and others added 9 commits February 24, 2022 15:13
Co-authored-by: João Moreno <mail@joaomoreno.com>
Co-authored-by: João Moreno <mail@joaomoreno.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks marked this pull request as ready for review February 24, 2022 18:05
@babakks
Copy link
Contributor Author

babakks commented Feb 26, 2022

Besides this PR's main focus, there's one other thing that I don't quite get it.

Here's the typical scenario that happens: when the Publish to Github command is triggered, which is the only command registered by the extension, it always creates a new repository (here) for the user and then pushes files to it. If the push fails, thenGithubPushErrorHandler comes in and, after some primary checking, tries to create a fork from the repository.

Now, my question is, why do we try to fork the repository? We've already created the repository itself. To me, this seems like a general way of handling push errors, so I checked for any other usage for the GithubPushErrorHandler but couldn't find any. Am I missing something?

To verify my point just check the publishRepostory() method (here).

@joaomoreno @alexr00

@joaomoreno
Copy link
Member

Now, my question is, why do we try to fork the repository? We've already created the repository itself. To me, this seems like a general way of handling push errors, so I checked for any other usage for the GithubPushErrorHandler but couldn't find any. Am I missing something?

Yes, if you clone a repository for which you have no permissions to push, this error handler comes in handy and prompts you to fork the repository. Catching a push error is more about that scenario than the Publish to GitHub scenario.

Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

It's very cool that you've added tests... even though they feel a bit overkill.

I've merged main and cleaned the implementation a bit, so we don't create functions all the time. Doing that, I found an issue. On case insensitive systems (ie Windows), a pull_request_template.md file will appear twice: once as pull_request_template.md, and once as PULL_REQUEST_TEMPLATE.md. It would be best to rely on fs.readDirectory on all detection; that way we'd always get what the OS has, instead of assuming file name casing.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
Copy link
Contributor Author

babakks commented Mar 10, 2022

Yes. I had missed the case sensitivity point too. I've updated the code and replaced fs.stat with fs.readDirectory.

@babakks
Copy link
Contributor Author

babakks commented Mar 10, 2022

Speaking of case sensitivity, the new testWorkspace directory contains both pull_request_template.md and PULL_REQUEST_TEMPLATE.md. This can be troublesome for people who clone the repository on Windows. So, I think it's better to keep one of the files/folders in the testWorkspace. Do you agree with me?

@joaomoreno
Copy link
Member

Yes, best to remove one of them.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
Copy link
Contributor Author

babakks commented Mar 13, 2022

Done. I've deleted lowercase counterparts.

@babakks babakks requested a review from joaomoreno March 24, 2022 17:47
@joaomoreno joaomoreno added this to the April 2022 milestone Mar 25, 2022
@joaomoreno joaomoreno merged commit 7fc5526 into microsoft:main Apr 1, 2022
@joaomoreno
Copy link
Member

Thanks! 🍻

@joaomoreno joaomoreno added the github Github extension label Apr 1, 2022
@babakks babakks deleted the handle-pr-templates branch April 2, 2022 09:28
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
github Github extension
Projects
None yet
3 participants