search

nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.27k stars 322 forks source link

Go: Add missing +build and go:build comments #1055

Closed danielledeleo closed 5 months ago

danielledeleo commented 5 months ago

A RHEL 8 test was failing because it uses go1.16. The old style must be retained for backwards compat.

tarynmusgrave commented 5 months ago

I know we are still working through these and looking through the PR history we haven't been sticking to this but the Git Style Guide for this repo mentions using past tense language for example 'added' instead of 'add' for the subject line.

ac000 commented 5 months ago

Personally I dont use past tense as it generally reads strange to me (I did initially in this project, until it grated on my brain enough), I prefer to use the more common imperative form where you give orders to make changes.

So for me the above summary is exacly how I'd write it...

ac000 commented 5 months ago

If this issue was introduced by https://github.com/nginx/unit/commit/9a36de84c8534cdd8d22b33d90309519e03abf3b, then perhaps you could add a Fixes: tag (see 763396b8be07be41b1baf336952fd222cbeb8db7 as an example) at the footer of the commit message (separated by a blank line) such as

Fixes: 9a36de84c ("Go: Use Homebrew include paths")

You can also add my

Reviewed-by: Andrew Clayton <a.clayton@nginx.com>

(Feel free to add your Signed-off-by: also...)

tarynmusgrave commented 5 months ago

Personally I dont use past tense as it generally reads strange to me (I did initially in this project, until it grated on my brain enough), I prefer to use the more common imperative form where you give orders to make changes.

So for me the above summary is exacly how I'd write it...

I don't have a strong opinion either way however it would be nice we were were consistent - in either direction. Should we be changing the Git Style Guide in that case? We don't want to make it more difficult for new contributors.

ac000 commented 5 months ago

Should we be changing the Git Style Guide in that case?

I would say yes (no surprises there!). There are also some things which just don't translate into past tense...

On the other hand, we do get contributions from non-native English speakers so unless we want to be re-writing everything into perfect English prose we need to allow some leeway. As long as the information is there and it's understandable...