xoreos / xoreos-tools

Tools to help the development of xoreos
https://xoreos.org/
GNU General Public License v3.0
66 stars 28 forks source link

NCSDIS: Made --list the default #12

Closed NAAleks closed 6 years ago

NAAleks commented 6 years ago

Just changed the kCommandNone to have the same behavior as kCommandListing. Added it as its own separate case for readability.

DrMcCoy commented 6 years ago

Neat, thanks! :)

Two things though:

So in this case, the first line of the commit message could be "NCSDIS: Made --list the default", with further explanation and that it closes a particular issue below.

NAAleks commented 6 years ago

Alrighty, you might need to give me a moment to figure out how to amend a past commit message.

DrMcCoy commented 6 years ago

(If you could add a comment when you've changed a PR, that would be great. GitHub doesn't send notification on a changed PR in all cases, so I won't necessarily get notified without a comment.)

DrMcCoy commented 6 years ago

The format of the commit message still isn't correct, though.

It needs to be something like

NCSDIS: Make --list the default

Change the kCommandNone to have the same behavior as kCommandListing.
Add it as its own separate case for readability.

Closes #11.

Yeah, I know, that feels a bit nit-picky, but it really helps keep the history readable for the future.

NAAleks commented 6 years ago

Oh, my bad. I understand the reason behind it; I just blanked out on the format for a second. I went ahead and changed it.

DrMcCoy commented 6 years ago

That looks better, yes, thanks! :)

There's two questions left:

The email address in the author field of the git commit doesn't match any of the email addresses you gave GitHub. That means that GitHub can't connect the commit to your GitHub account. And the email address in the committer field is different still (but that's not really important, because when I merge this PR, the committer will be overridden with my name and address).

That's why https://github.com/xoreos/xoreos-tools/pull/12/commits/284380f7d9f33d5eee4085733ec13bd918d32e2a here says "Alex authored and Alex committed", and none of these are linked to your GitHub acccount. Is that on purpose, or do you want to change that before I merge the PR?

You can view that information with git log --format=fuller, if you're using the usual git command line tools.

Also, how do you want to be credited in the contributors list in our AUTHORS file? :)

The format is usually Name <email address> or Name (nickname) <email address>.

NAAleks commented 6 years ago

I went ahead and made the changes. It should say "Alex Authored and NAAleks committed."

DrMcCoy commented 6 years ago

Erm, if you want the NAAleks to be visible when merged with xoreos-tools, that's the wrong way round. You'd want the Author email address to be your GitHub email address, not the Commit. Since I'm going to merge it, the Commit field will be overwritten with my name and email address.

(Also, the email address is what's important. So it can still say Alex in the Author field, just with the ncsu.edu email address.)

As is now, the commit won't be attributed to you when merged into xoreos-tools, but to Alex with the .local email address.

NAAleks commented 6 years ago

I'm sorry for the hassle, you can tell I'm just getting to know this side of git. I went ahead and changed the author.

DrMcCoy commented 6 years ago

It's entirely fine, we all started somewhere. :)

Merged it as b8c3340, thanks! :)

If you want to continue fixing things for ncsdis, there's another thing: The format of the help text, or how the options are printed to be specific, isn't quite what I'd like to be.

I'd like the --help and --version lines to come first (as they are already), then a blank line, then three possible modes (list, assembly, dot), then a blank line, the options for the individual games, a blank line, and then the control option.

I.e. each "section" of options (information about the program, modes, games, minor switches) should be separated by a blank line.

NAAleks commented 6 years ago

Yeah, sounds easy enough. A few questions though, do I need to create a new fork and do I need to create an issue first?

DrMcCoy commented 6 years ago

Nope, no need to create a new fork.

You can do this in an own feature branch in your fork, if you want. Or you can use the master branch. The former is advisable if you want to work on multiple things at once.

And no real need to create an issue for it either.