-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update README.md and split into several related files #452
Conversation
I removed as much incorrect/outdated info as I could, but there were several things I wasn't sure about, and I'm sure I missed some. Keep an eye out for anything I mistakenly changed or misunderstood... 🤷 |
Thanks @ThanGerlek! I'll review this when I have a moment, and I requested a review from @19mdavenport on this too since he knows more about it than I do. Quickly though, you may noticed that I added a new line to the end of your PR description. It contains a special closing keyword which GitHub recognizes to directly link this PR to the issue you opened earlier (great job BTW). These closing keywords only work when placed inside the PR description (putting it in a comment won't do the trick). You'll also notice that I added a link to the issue in #444 which is the parent issue of these kinds of changes. Previously, this kind of adjustment had been reference by text only, but I hadn't yet created an issue for it. Thank you for doing so! |
Upon closer examination of the sources, I understood why you had not wanted this to close the #451 issue: You intend for future work to actually clean up the content whereas this PR mainly affects the organization of the files. I removed the closing keywords from the PR description accordingly. |
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.
This structural change looks great, @ThanGerlek, and I really appreciate the expanded and clarified content along the way!
I've provided several comments in files and lines of code with ways we may be able to improve the presentation of information.
After working through all the content, I realized that I ended up proposing two opposing ways of dealing with the platform-specific setup instructions. Both sets of comments remain in the review, but I'll clarify which I recommend. The opposing paths are:
- Remove unused/unneeded setup instruction files (recommended)
- Change content of filler files (not recommended)
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.
These changes are looking great!
There's a presentation change I want to make; but otherwise, a big thank you for making these changes happen!
…truction240/autograder into 451-improve-readmes
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.
This looks good to me, but I would wait for a review by @19mdavenport before merging it.
What else would be needed in order to actually close #451? (Have we made the adjustments already; if not, can you specifically update #451 to indicate what work remains?)
@ThanGerlek FYI It seems that we keep fighting over trailing whitespace on the lines. Can you make sure you have these "On Save" settings enabled?
|
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.
OK, now I'm fully comfortable with the presentation of the content. I feel like the information is a readable as it can be, and I'm happy with the way it presents on the screen.
I did chat with @19mdavenport this morning, and he mentioned that there may be some content adjustments we need to make. Let's wait to hear back from him— he's a busy man, but he'll look at this soon enough.
Just a small thing, GitHub treats the |
Great suggestion, @webecke! This is accounted for in 7423999. |
a6f01a2
to
5cb3786
Compare
5cb3786
to
e9cc336
Compare
Also marking this as resolving #335 since the clarifications and adjustments to the language indicate no further requirement to have Canvas API tokens. |
I consolidated the development and getting started sections of the README.md into one small section and moved it to the top. If we're opposed to that we can change it back, but for almost everyone coming to look at the Autograder repo the info they need is in the Getting Started Guide and Contribution Guide. Thus I propose they should be at the top. |
Updated the PR description to represent recent changes. At this point this PR closes #452, although not until we're all happy with it, so it's effectively still a WIP. As such I've changed it to a draft PR as well. |
@19mdavenport I think you're review is the last thing we're looking for on this PR. @ThanGerlek Are you aware of any other changes we're still waiting for? |
Nope, that's all. Not just Michael's review but also any additions he thinks might be useful |
I went ahead and marked it as "ready to review" since you have contributed everything you are planning on for this PR. Other reviewers and contributors may weigh in, but now is the time for that process to be happening. (Typically, the "draft" stage is a signal that other people should not look at this PR because the PR author is still working on critical aspects of it.) |
I vote we go ahead and merge this in if no one else has things to add. @frozenfrank @ThanGerlek @19mdavenport |
Full support, brother! Since the latest significant changes were 2 weeks ago, at this point, if @19mdavenport wants to change anything he can submit a new PR. |
A PR for #451. Adds, updates, clarifies, and reorganizes "getting started" info for new devs joining the autograder team.
README.md