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

Set correct code blocks language in README.md #329

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PeterDaveHello
Copy link
Contributor

This will give the code blocks in README.md syntax highlighting for better readability.

@codecov
Copy link

codecov bot commented Dec 20, 2020

Codecov Report

Merging #329 (b14afff) into master (b289752) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   71.59%   71.59%           
=======================================
  Files           8        8           
  Lines         514      514           
=======================================
  Hits          368      368           
  Misses        146      146           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b289752...b14afff. Read the comment docs.

@rafaelpivato rafaelpivato self-assigned this Dec 20, 2020
@rafaelpivato
Copy link
Contributor

Thanks for this. There is one block marked with bash which contains only a banner, no bash call. Also, there are some commands with results dumped in separate blocks like this:

safety check --bare
django

Instead of this:

$ safety check --bare
django

Finally, I am not sure whether these are all bash calls. Some are console sample outputs, not bash scripts. I'll have another thought about this later. My preference would be to merge a final fix to all these minor issues.

@PeterDaveHello
Copy link
Contributor Author

PeterDaveHello commented Dec 20, 2020

I use bash because most of them(which are not changed at this PR) are already using bash, for consistency reason, I use bash, not sh here, but actually it's the same as sh on GitHub if I didn't recall wrong.

Looks like the case you mentioned (safety check --bare) doesn't seem to be in the scope of the changes here? The changes here all affect the visual result, because there is shell pipe(|) in it, and the change can make them to be more readable.

@rafaelpivato
Copy link
Contributor

We appreciate your effort looking forward small details like this. I would urge you to bring more complete pull-requests for similar areas. In the meanwhile, I think we are good the way it is. If you want to bring #330 changes together with these and also align block usages across entire README, that would be meaningful.

@PeterDaveHello
Copy link
Contributor Author

It's odd that this is not meaningful, both #329 #330 were marked invalid and closed by you which I can't reopen, can you open one of them so that I can revise it?

With all due respect, this is a little bit contributor unfriendly, it'll be really great if there could be more discussion before the mark an closure ;)

@rafaelpivato
Copy link
Contributor

I hear you, @PeterDaveHello 🌻

Looks like we need better guidelines for contributors indeed. At this moment you can consider just grouping related pull-requests into a single one. That's my main point here.

The benefit you brought with the Docker image size reduction was worth going through multiple spread pull-requests, even though I would expect them to be grouped. Anyway, for this case, I don't see a value in doing that.

Finally, I honestly don't know which one I should re-open, so, please, if you want this to be considered again, open a new pull-request with entire README reviewed.

@PeterDaveHello
Copy link
Contributor Author

Thanks, maybe just reopen this one as it's sent first? I'll update the commit in it once it's opened ;)

@PeterDaveHello
Copy link
Contributor Author

Hi @rafaelpivato, do you have a minute help reopen this PR so I can push revised commits here? Thanks.

@PeterDaveHello
Copy link
Contributor Author

PR updated! Thanks!

@PeterDaveHello
Copy link
Contributor Author

@rafaelpivato, is there anything else I can do to get this merged :)

@rafaelpivato
Copy link
Contributor

Sorry. at this moment we are just being strict about priorities in general. The PR looks good. We should merge it after I have time for a small review. Thanks for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants