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

add sys/select.h to includes #99

Merged
merged 1 commit into from Feb 23, 2017
Merged

add sys/select.h to includes #99

merged 1 commit into from Feb 23, 2017

Conversation

dcat
Copy link
Contributor

@dcat dcat commented Feb 23, 2017

add sys/select.h to includes

@rofl0r
Copy link
Contributor

rofl0r commented Feb 23, 2017

your commit message describes what you're doing, but not why you're doing it. missing critical info.

@dcat
Copy link
Contributor Author

dcat commented Feb 23, 2017

well... because termbox.c uses select().

@rofl0r
Copy link
Contributor

rofl0r commented Feb 23, 2017

it's still missing in the commit message. (hint: some ppl try to figure out what happened in a repo by reading commit messages. in your case they have the look at the code additionally to figure out why)

@dcat
Copy link
Contributor Author

dcat commented Feb 23, 2017

rebased the commit message to be single-line, hopefully now you'll be able to read it

@rofl0r
Copy link
Contributor

rofl0r commented Feb 23, 2017

the problem is not that it wasn't single line (albeit the first line "update foo.c" was a bit too generic). it's that it's missing the rationale, still.
so a good commit message would tell a good summary what it's doing in the first line,
and then go on to explain the changes and the rationale for the changes in the next lines.

@dcat
Copy link
Contributor Author

dcat commented Feb 23, 2017

wait, you want me to explain the rationale between adding a dependency?

@rofl0r
Copy link
Contributor

rofl0r commented Feb 23, 2017

i don't want you to do anything, since this is not my repo. however if it was mine, i'd request a commit message explaining the "why".
for example:

foo.c: add missing include sys/select.h

the code uses select(), but the header defining it was not included.
this broke compilation on AIX.

@dcat
Copy link
Contributor Author

dcat commented Feb 23, 2017

well, let's hope the author isn't as autisticly pedantic as you are.

@nsf nsf merged commit 355cccf into nsf:master Feb 23, 2017
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

3 participants