amiaopensource / ffmprovisr

Repository of useful FFmpeg commands for archivists!
https://amiaopensource.github.io/ffmprovisr/
527 stars 65 forks source link

Creating a style guide for contributors? #429

Closed kieranjol closed 4 years ago

kieranjol commented 4 years ago

Hi folks - I was chatting to @DaleLore about this after the Pull Request https://github.com/amiaopensource/ffmprovisr/pull/428 was merged and it got me thinking that we should have some sort of style guide and contributing advice. I don't think that we have much style advice for contributors, and it would be best for all if we could present this in advance rather than letting people know during a code review. I'm not sure what form this should take, but I looked at a bunch of recipes and picked up on a bunch of trends that I think we'd all mention when doing a code review, so best to have them out front in advance. Perhaps a basic list of principles would be something like:

kfrn commented 4 years ago

Good call! Great list, @kieranjol - comprehensive, I can't think of anything to add offhand.

I'd be inclined to add this info in the readme under the Contributing section, and link to it (or repeat the relevant parts) in a PR template. What do you reckon? Happy to set that up.

kieranjol commented 4 years ago

I think that's the place for it alright, thank you, that would be great!

ablwr commented 4 years ago

This is great!

ablwr commented 4 years ago

This will definitely help with updating other scripts to conform to these best practices. When I was talking with @DaleLore about the code review, I admitted that most of our other scripts don't use this concept of variables for users to fill in -- like the GIF script, for example, should probably be updated to not be so academic and just be practical (e.g. loop forever, reasonable sizes, etc). I'm sure most of the scripts could benefit from this style guide and get some updates.

I can't think of anything HTML-specific other than following the template, and I know this still needs a semantic HTML overhaul but it's a pretty big/daunting task that I haven't felt the urgency to pick up.

retokromer commented 4 years ago

Regarding HTML, there would be a possibility like Tommy did for his FFCommand_Engine, but I guess that would be an overkill here. [Not sure “overkill” is politically correct these days.] In my opinion the template should be sufficient.

ablwr commented 4 years ago

@retokromer Would you consider opening a PR that adds the FFCommand_Engine as a sister project in the README?

kieranjol commented 4 years ago

@ablwr, this is a great idea re revising older recipes, having a frequently updated style guide could also be a kind of signpost for low hanging fruit contributions from newer users.

retokromer commented 4 years ago

@ablwr Yep. And also a separate page about FFCommand_Engine, right?

ablwr commented 4 years ago

I don't have a very strong opinion about this, but to me that seems a bit out of scope, and up to FFCommand_Engine to provide. A link to that project should be sufficient, in my opinion.

retokromer commented 4 years ago

Okay, I will open a PR with only the link.

kieranjol commented 4 years ago

Aye, i agree with @ablwr on scope.

retokromer commented 4 years ago

No problem at all! I will publish the compiling guide (possibly I am going to program a multi-platform Makefile) and the way to personalise the presets on my own website. At the moment these hints are only “somewhere” in my twitter feed…

privatezero commented 4 years ago

West coast late to the chat as usual, but agree with above about scope

kfrn commented 4 years ago

This will definitely help with updating other scripts to conform to these best practices. When I was talking with @DaleLore about the code review, I admitted that most of our other scripts don't use this concept of variables for users to fill in -- like the GIF script, for example, should probably be updated to not be so academic and just be practical (e.g. loop forever, reasonable sizes, etc). I'm sure most of the scripts could benefit from this style guide and get some updates.

Yeah, I was thinking this - there are a bunch of scripts that could probably do with a second pass, especially re: hardcoded variables.
I'm happy to have a look over it, but I can't promise haste either 😄

kfrn commented 4 years ago

I can't think of anything HTML-specific other than following the template, and I know this still needs a semantic HTML overhaul but it's a pretty big/daunting task that I haven't felt the urgency to pick up.

Yeah, same. We've talked about it a couple of times before 😅 and there's an issue here: https://github.com/amiaopensource/ffmprovisr/issues/397 One day ...

kfrn commented 4 years ago

I believe that this is now resolved! I'll close the issue for now - feel free to re-open if we need further discussion :)