Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Feb 1, 2018

This is a quick shot not to derive from c-mode but using cc-mode functions. This is more a proof of concept than a thought through solution. I have tested it on a few files an I haven't found any misbehaviour for now. Maybe this can be elaborated upon? Some of the existing php-mode code maybe unnecessary now which worked around the limitations of being a derived mode from c-mode.

@zonuexe I will surely update this branch removing such code parts and fixing any issues I find while using it, so don't feel pressured merging it quickly. If you have time reviewing it, maybe you can give your ideas, too?

Github Issue: #407

@ghost ghost mentioned this pull request Feb 2, 2018
@ejmr
Copy link
Collaborator

ejmr commented Feb 3, 2018

Thank you for the patches. Coincidentally I was looking at the project tonight for the first time in weeks (months?) and saw this pull-request and the related issue. Just two comments I wanted to make in passing:

  1. Commit 3b688f6 ("derive from prog-mode not from c-mode") must have a detailed commit message. Right now it probably looks obvious to anyone paying attention as to why this change is useful and desirable. But months from now no one will remember issue php-mode executes c-mode hooks #407 or the discussion around it, and anyone looking through the output of git log will be confused as to why a fundamental change to PHP Mode has no accompanying justification. TL;DR: This is the type of change where the commit message must adequately answer the question, "Why?", for the benefit of all future developers.

  2. You mention that "some of the existing php-mode code may be unnessecary" with this change in place. I am ninety-nine percent sure you are correct; if PHP Mode derives from a different mode then I am sure there are existing work-arounds to CC Mode related bugs that can then be removed. However, with that said, I would personally suggest to you and @zonuexe to address that issue in a separate branch after doing everything needed to merge this one. The years I spent on PHP Mode taught me that large, sweeping changes can easily bite you in the ass later on, heh. It will be easier to manage these changes and maintain the project as a whole if the aforementioned unnecessary code is removed in an incremental fashion, instead of as part of a larger, sweeping change.

Anyway, those are just the two things that came to mind. Thank you for the pull-request. And my thanks in advance to @zonuexe for handling the merging of this and any related changes. It has made me very happy tonight to look over the project and see all of his time and effort spent maintaining PHP Mode.

@ghost
Copy link
Author

ghost commented Feb 6, 2018

@ejmr thanks for the kind words and the knowledge :)

A few questions:

  1. How would you change the commit message? Is it possible in GitHub somehow or is a push --force the only option (which is not nice but wouldn't cause any damage I believe)?
  2. Have I understood you correct to merge this pull request soon and do all the backtracking work with code that can be removed later in a seperate branch only merging small steps into php-modes master branch?

@ejmr
Copy link
Collaborator

ejmr commented Feb 6, 2018

  1. I don't know if it possible to change a commit message directly from GitHub. Personally I preferred to use git commit --amend if I needed to edit the most recent commit on a branch, or git rebase if I needed to edit an earlier commit. Regardless, modifying any commit in any way like that, whether through GitHub's UI or not, would --forceibly change a branch/pull-request. You are correct that such an action would not cause any damage; the exception would be if a person has created a branch forked from your branch, but in that hypothetical situation that person has already erred by forking an umerged pull-request in the first place. As for how I would change the commit message specifically, in terms of content, all I meant is that message needs to explain why the patch does what it does. For example, consider commit 3ecdd48; the message describes not only what the patch does but why it exists at all by providing examples to justify how it's an improvement, and most importantly, attempts to be comprehensive to the point that the justification is easily understandable two-and-a-half years later in the complete absence of any other context. Looking at a change from that perspective can help a lot when it comes to writing good commit messages, e.g. thinking, "If someone three years from now were looking at this wondering why we change c-mode to prog-mode in this patch, how would I explain that change to them?"

  2. Yep, exactly, heh. But that suggestion was more for @zonuexe and was undoubtedly unnessecary, since he maintains the project well without unprompted input from me, heh.

Deriving from c-mode results in a few unwanted side effects like the
execution of the c-mode-hook everytime php-mode is enabled.  Deriving
from a major mode makes only sense if the mode is a specialization of
the derived mode (like ng2-mode deriving from typescript mode as it
adds only features and it makes sense that the typescript hook is
being run).  Also there could be some workarounds for c-mode specific
issues which can be removed now (but haven't for the most now)

This commit does not touch the usage of cc-mode stuff like
c-lang-defconst and c-add-language.  Instead of deriving c-mode to
inherit the syntax table c-populate-syntax-table is called directly.

The only workaround removed was the defcustom var
php-do-not-use-semantic-imenu which was a workaround for a broken
imenu when using semantic-mode (as c-mode enables semantic-imenu and
the setting was inherited) which is not necessary anymore.

Github Issue: #407
@ghost
Copy link
Author

ghost commented Feb 7, 2018

FYI: I rebased the commits and updated the commit message.

@ejmr
Copy link
Collaborator

ejmr commented Feb 8, 2018

That new commit message is really well-written and would make a great example in the future of the level of quality that should be the standard (in my opinion). Thank you very much for the addition.

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