-
Notifications
You must be signed in to change notification settings - Fork 516
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
add command to preview a PR #1677
Conversation
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.
I am not convinced about this. I'd rather go back to the drawing board a bit more first.
First of all, my biggest fear is that we're taking on too much and too much can go wrong.
It is after all a very niched abstraction. You've assumed that it's always about checking out from mdn/content and that alone misses that someone might want to check out myfriendsfork/content
.
All in all, I'm scared that we're taking on the task of teaching people the basics of git and we're also taking on other git related tools.
Also, this is quite a big change that edits a lot of code which can break things and it's a "distraction" towards the Yari1 blockers which we need to focus on first.
In general, I'd like to start from the absolute minimum and work our way up slowly.
It could just start with a blurb in the README, like "How to check out someones PR locally" and we explain how you do it (without using merge
I hope!). And if that's not enough, we can write a gist script that you can download on the side.
Lastly, if we do anything to hold people's hands with using git
I think I'd rather build it without a UI first and then discuss and consider using Web or CLI once things mature.
return UPSTREAM_URLS.includes(url); | ||
}); | ||
} catch (e) { | ||
throw new Error(`unable to determine upstream: ${e}`); |
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.
throw new Error(`unable to determine upstream: ${e}`); | |
throw new Error(`unable to determine upstream: ${e.toString()}`); |
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.
Alternatively ${e.message}
?
There's a lot of great potential in here. If done correctly, we can write some routines that can be accessed via a CLI or even via the "Writer's Homepage", similar to the list of recently viewed docs. Imagine if the Writer's home page could have a searchable drop-down of all open PRs and you just click the one you want and Yari can automatically execute the necessary checkout commands and voila, you have a anybody's PR on your localhost:5000 without having to type anything. Are you interested in refreshing this? In particular, we'd need to make it work for mdn/content and mdn/translated-content and we ought to make the CLI as thin as possible. |
@fiji-flo I wonder if we could instead recommend the GitHub CLI to people? https://cli.github.com/ - It is available for all platforms(https://github.com/cli/cli#installation) and has a number of super useful tools such as checking out a PR locally( |
I do have mixed feelings regarding the github cli. I'd add something to the README like this: Assuming you have run this once:
You can run:
Or use the github cli. |
I can add some docs for that. |
Closing in favour of #5202 |
yarn tool view-pr https://github.com/mdn/content/pull/76 yarn run v1.22.10 $ node tool/cli.js view-pr https://github.com/mdn/content/pull/76 preview MDN/Kitchensink → http://localhost:5000/en-us/docs/MDN/Kitchensink ✔ checked out PR: https://github.com/mdn/content/pull/76 Don't for get to run git checkout main when done!
Can be improved but maybe something to start with and to get some feedback @chrisdavidmills