-
Notifications
You must be signed in to change notification settings - Fork 57
Add suggest edits to top repo button #25
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
Conversation
🚀 📚 Preview for git commit SHA: 0bbd15e at: https://60660aa09d8db72173790900--pythia-foundations.netlify.app |
Hi @dcamron , I'm still not totally clear of the right collaborative workflow here ... so if I wanted to make a change to your fork, would I first create a fork of your fork and then attempt to make a pull request to that? |
The easiest path is probably to install the GitHub Command Line Tools so that from your copy of the main repo you can do: gh pr checkout 25 The full manual way is to add Drew's fork as a remote and check out his branch: git remote add dcamron [email protected]:dcamron/pythia-foundations
git checkout --track dcamron/suggest-edits Either way, after that make changes, commit them, and then do: git push |
I just successfully pushed to this branch, yay! My commit added some verbiage to the footer directing readers' attention to the new buttons, and also says something about this on the "How to Contribute" page. |
🚀 📚 Preview for git commit SHA: 9603fca at: https://6066182dc94c7b3821994331--pythia-foundations.netlify.app |
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.
All good and ready to merge, but maybe others want to "practice" pushing to this branch first!
Tried to push but encountered linting error once again ... seems that although I checkout out pr 25, my _build directory still had the Cartopy content I had added. |
(reads the error messages closer) Actually the linting worked; the error lies in the creation of the environment. |
🚀 📚 Preview for git commit SHA: b477bbc at: https://6067602db3f8a416aa71f574--pythia-foundations.netlify.app |
@ktyle as you noted above, the old build was not the issue here, but FYI you can always do
to wipe out previous builds. |
@dcamron said
This raises a workflow issue. In general when we open new Pull Requests, it's probably best that they are from new feature branches of our fork, not from |
@brian-rose For git, one branch is no different than any other-- Having said that, it's a really bad idea to be making PRs from the main branch on your fork in general. The problem is if on your laptop, you're making all your commits on TLDR; yes it's better to work on your own branch other than |
@dopplershift thanks for that, I think I "git" it |
Hmm. Struggling to figure out how to use suggestions. After cloning dcamron:suggest-edits and rendering a local copy of the book, I opened the book, clicked "suggest edit", made my change, and then I'm given two options:
Which do I want? I tried (1) but that seemed to actually commit the change to main (just like it says), and gave no indication that I could see that this was a suggested change (it looked like an actual change). Thanks for any guidance. |
I'm not sure, but I suspect that option (1) only shows up if you already have write privileges in the source repo. What happens if someone who is not part of the dev team clicks the "suggested edit" button? |
(1) only works since @clyne has write access. (2) would be the better way to "suggest a change", and would be the only option available to someone from the broader community. That method works pretty well for things like editing docs, where the needs for tab completion and code highlighting aren't as great as when editing code. |
@dopplershift Good to know. Thanks. but with (2) my one-line change is failing: https://github.com/ProjectPythia/pythia-foundations/pull/29/checks?check_run_id=2271950198 |
Probably best to complete the discussion over there, but nothing went wrong with the process--it's just that your change is failing linting. 😉 |
This seems like an important infrastructure topic. The linter is very stringent. My hope is that we can set up a workflow that is a little more encouraging of community contributions! |
Well the linter is able to fix that automatically for you when you make a commit locally. Heck, most editors automatically add a newline when you save. The problem is that if you do it through the GitHub web interface, it doesn't add it nor can it run the linter. I have no idea what config tweaks exist on the linter to make this easier or if there's any way to get it to annotate the PR to identify the problematic lines. |
Yeah, and I wonder whether this will just cause more rather than less confusion for a newbie who is trying to contribute. An alternative is to remove the "suggested edit" button and instead offer detailed instructions on how to branch, edit locally, and open a new pull request. We need to do this anyway! |
I think suggestions as they currently work will cause confusion and frustration. Perhaps offering multiple ways to make suggestions would be best: GitHub discussions for the impatient, or branch/edit/PR for the adventurous |
Another option would be to let contributors do like @clyne did and someone with write permission can correct any style issues by pushing to their branch. |
Merging now, based on our discussion at last week's EWG! Sorry I meant to do this earlier. |
Jupyter book has a built-in extra dropdown button for "suggest edits" per our discussion in today's meeting. This is also a chance for anyone in @ProjectPythia/education to see if you're able to push to my PR. I can always wipe out your change if it works. Attached is a screenshot of the important setting beneath the body text of your PR to enable collaboration from project maintainers. This is enabled by default for me, and if it's disabled then external maintainers will not have default write access to your branch on your fork. Otherwise we might have a permissions issue somewhere? Screenshot attached: