safety icon indicating copy to clipboard operation
safety copied to clipboard

Set correct code blocks language in README.md

Open PeterDaveHello opened this issue 4 years ago • 11 comments

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

PeterDaveHello avatar Dec 20 '20 14:12 PeterDaveHello

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.

codecov[bot] avatar Dec 20 '20 14:12 codecov[bot]

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.

rafaelpivato avatar Dec 20 '20 17:12 rafaelpivato

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.

PeterDaveHello avatar Dec 20 '20 17:12 PeterDaveHello

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.

rafaelpivato avatar Dec 27 '20 14:12 rafaelpivato

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 ;)

PeterDaveHello avatar Dec 27 '20 15:12 PeterDaveHello

I hear you, @PeterDaveHello :sunflower:

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.

rafaelpivato avatar Dec 27 '20 15:12 rafaelpivato

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

PeterDaveHello avatar Dec 27 '20 15:12 PeterDaveHello

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

PeterDaveHello avatar Dec 30 '20 14:12 PeterDaveHello

PR updated! Thanks!

PeterDaveHello avatar Jan 05 '21 15:01 PeterDaveHello

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

PeterDaveHello avatar Jan 27 '21 05:01 PeterDaveHello

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.

rafaelpivato avatar Feb 07 '21 19:02 rafaelpivato