Skip to content

Minor cleanup #368

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

Closed
wants to merge 1 commit into from
Closed

Minor cleanup #368

wants to merge 1 commit into from

Conversation

sergeyklay
Copy link
Contributor

No description provided.

@@ -24,7 +24,7 @@

;; This file is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
Copy link
Member

@zonuexe zonuexe Jul 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two spaces after period should be kept.
Please use purcell/flycheck-package: Flycheck checker for elisp package metadata.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zonuexe You beat me to it, lol.

@ejmr
Copy link
Collaborator

ejmr commented Jul 29, 2017

Thanks for the pull request and patch. Two comments though:

  1. Please write a more descriptive commit message. "Update php-mode.el" could mean literally anything. Ideally a person should never be forced to look at the code to see exactly what a patch does, because the commit message should always describe that in detail.

1.a. If a patch fixes typos, adds or updates documentation, corrects information in tr he README---and so on---then the commit message needs to make that clear. Basically the commit message needs to say something along the lines of, "This patch only fixes a few typos and does not affect any behavior in PHP Mode."

  1. Thank you for fixing the "mode" typos. But please restrict the patch to only those typos. Or to put it another way, do not change sentences from using two-spaces after the period into sentences which only use one space. And do not change the indentation of code which the patch does not affect. The same goes for adding more blank lines. The problem with doing this is that in the future developers could easily get the wrong idea and think there was a meaningful change in a piece of code where in reality all that changed was the indentation. It saves time and headaches in the future.

But other than that we should definitely use "PHP Mode" consistently with Mode capitalized. If you could please restrict the patch to doing only that then I will definitely merge it.

@sergeyklay
Copy link
Contributor Author

Close in favor of #369

@sergeyklay sergeyklay closed this Jul 29, 2017
@sergeyklay sergeyklay deleted the patch-1 branch July 29, 2017 19:39
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.

3 participants