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

unit1612: Fixed the compilation of the new unit test #5024

Closed
wants to merge 1 commit into from

Conversation

captain-caveman2k
Copy link
Contributor

Follow on from 3f74e5e. Due to a typo in Makefile.inc unit1611 was used as test1612. Once corrected there were a few minor compilation issues to fix.

@jay
Copy link
Member

jay commented Mar 3, 2020

unit1612: Fix the compilation of the new unit test

@bagder
Copy link
Member

bagder commented Mar 3, 2020

if we'll be very strict, I would also prefer if we dropped the initial uppercasing of the first letter and when possible rather spell out the actual fix rather than saying "fix". Perhaps:

unit1612: compile the correct source file for the test

@jay
Copy link
Member

jay commented Mar 3, 2020

I think as a requirement it's probably too strict. Keywords like fix/add/remove or whatever may help communicate an overall effect (which is then fully spelled out in the commit message), and the initial uppercase style can be found in the history occasionally. I have some bias. Take for example

curl_getenv.3: Fix the memory handling description

- Tell the user to call curl_free() to free the pointer returned by
  curl_getenv().

It could be done with some limitation in the subject as

curl_getenv.3: tell the user to call curl_free() to free the pointer

I think the first one communicates better. I'm not sure if everyone shares that view.

@bagder
Copy link
Member

bagder commented Mar 3, 2020

I didn't mean it as a requirement. I just offered my take on what would make the commit message even better in my view.

Follow on from 3f74e5e. Due to a typo in Makefile.inc unit1611 was
used as test1612. Once corrected there were a few minor compilation
issues to fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants