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

Fixes https://github.com/tmewett/BrogueCE/issues/45 #52

Merged
merged 1 commit into from
Feb 4, 2020
Merged

Fixes https://github.com/tmewett/BrogueCE/issues/45 #52

merged 1 commit into from
Feb 4, 2020

Conversation

AndrewSav
Copy link
Contributor

No description provided.

@tmewett
Copy link
Owner

tmewett commented Feb 3, 2020

Thanks for fixing this! If I'm not being a pain here.. could you remove the sub-heading in the CHANGELOG (so it's just a flat list - sorry, I meant to set this up around 1.8.1..) Will merge after that. Additionally, I would suggest expanding the commit message to be more descriptive of the change when not on GitHub (you can put Fixes: #45 in the message body. See the contributing guide)

@AndrewSav
Copy link
Contributor Author

AndrewSav commented Feb 3, 2020

Sorry, not sure what you mean, could you please explain a bit more. It seems I'm missing some context. How GitHub commit message can ever be not on GitHub? Is it being copied somewhere? If yes where and by what process?

@tmewett
Copy link
Owner

tmewett commented Feb 4, 2020

I mean that when viewing the commits locally with Git, "fixes 45" isn't very descriptive, since I can't easily see what issue 45 is, like I can on GitHub (by mouse-over).

@AndrewSav
Copy link
Contributor Author

Is this more of a theoretical issue (e.g. you think that it might cause some problems), or is it a practical one (e.g. you actually tried it and it proved unworkable for you)?

I personally prefer to see commit history on GitHub, but even if I do this at command line I see this:

2020-02-05_08-44-01

Now I can simply ctrl-click the link and read the details. It does not seem too onerous?

@tmewett tmewett merged commit 873ed42 into tmewett:master Feb 4, 2020
@tmewett
Copy link
Owner

tmewett commented Feb 4, 2020

it's just a matter of what I personally consider to be good practice. I like to be able to know what a commit does from reading its message. But I am happy to merge

@AndrewSav
Copy link
Contributor Author

@tmewett understood, and thanks, I'll try to keep this in mind for the future,

@AndrewSav AndrewSav deleted the fix-issue-45 branch February 6, 2020 20:56
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.

2 participants