search

hayamiz / twittering-mode

An Emacs major mode for Twitter
http://twmode.sourceforge.net/
545 stars 92 forks source link

Add customize into twittering-mode #97

Open xmaillard opened 9 years ago

xmaillard commented 9 years ago

Hello,

I needed to move 2 small config options into my custom-file and for this, I had to bring all the customize stuff into twittering-mode.

Here is the result of my work. I imagine I am not alone preferring to keep an .emacs uncluttered so feel free to take it (or not). I'd be thankful to get feedback anyway.

Thank you Xavier

cvmat commented 9 years ago

Thank you for contribution! Especially, support for the customize interface and cleaning up docstrings are very awesome work which requires patience. I would like to import your work.

But, I am worried about the following points of the patch. How about them?

  1. The 537-th line on your version of twittering-mode.el seems to cause an error when displaying Twittering-mode group. https://github.com/xmaillard/twittering-mode/blob/2e5b337934e0ea1f8c98e5d35d87d3dd0bf68957/twittering-mode.el#L537 I think that the patch should be fixed as following.

    diff --git a/twittering-mode.el b/twittering-mode.el
index c8577de..2e02d6c 100644
--- a/twittering-mode.el
+++ b/twittering-mode.el
@@ -534,7 +534,7 @@ If the value is nil, doesn't show replied tweets."
                    :value nil)
             (const :tag "Show all replied tweets"
                    :value t)
-                 integer :tag "Number of replied tweets")
+                 (integer :tag "Number of replied tweets"))
:group 'twittering-mode)

(defcustom twittering-default-show-replied-tweets nil

  2. The patch replaces TABs for indentation with whitespaces. Is this an intended modification? The Emacs Lisp Coding Conventions, http://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html , includes the following sentence.

    • Indent the file using the default indentation parameters.

    And the default value of indent-tabs-mode for Emacs-Lisp mode is non-nil.

    Do you have some opinions of that?

  3. (optional) Your second commit https://github.com/xmaillard/twittering-mode/commit/2e5b337934e0ea1f8c98e5d35d87d3dd0bf68957 includes multiple purposes, support for the customize interface (with cleaning up docstrings), cleaning up trailing whitespaces, and replacing TAB indentation with whitespace indentation.

    Can you split them into 3 commits?

ayman commented 9 years ago

Is this different from what pull request #76 added with M-x customize-group and doc strings?

xmaillard commented 9 years ago

@ayman chances are my changes are partly in double with yours :/ Sorry about that.

xmaillard commented 9 years ago

@cvmat

Concerning point 1, you should be right. Sorry.

Concerning point 2, I was not aware enough of that particularity. I will change that here and submit something suiting better to the standard guidelines

Conerning point 3, If I remove all this "whitespace" thing, the patch will still be ~800lines. I will try to cut it in part and I will probably try to follow @ayman stuff -i.e change the defgroup to twittering (or, better: twit ?)

How can I redo my work technically ? git revert ?

Regards

cvmat commented 9 years ago

@ayman , I am sorry for not responding to your request. The Xavier's patch seems to include defcustom declarations for (probably) all of variables with appropriate types.

cvmat commented 9 years ago

@xmaillard I think that twittering-mode is better as a group name. LaTeX-mode, c-mode and other many modes define groups with its name without preceding -mode. But there are also groups with preceding -name such as diff-mode, eshell-mode, and so on.

How can I redo my work technically ? git revert ?

I am not so expert with git (I usually manage the repositories with hggit)... If I were you, I would try the following steps.

  1. Reset the repository. git reset 'HEAD^' The command restores the repository to the state just before committing the latest modification.
  2. Rearrange the patch and commit it.

  3. Make a new branch fixed-patch that includes the latest version.

    git checkout -b fixed-patch a4489344a68ef4068e344b3cc7d55dbb7ab6cd5e
git pull https://github.com/hayamiz/twittering-mode

  4. Cherry-pick your rearranged patch without commiting it.

    git cherry-pick -n FIXED-PATCH-REV

    where FIXED-PATCH-REV denotes the revision of the commit rearranged on the step 2. The above command will make some conflicts.

  5. Resolve conflicts (with hands or a tool like kdiff3, git mergetool) and commit it.
xmaillard commented 9 years ago

@cvmat it seems I am doing crap with git :(

I successfully re-arranged my patches and committed it:

commit dd3cfae9ad16cb7aef47dc5a5efa8387e1b9db0d Author: Xavier Maillard xavier@maillard.im Date: Mon Jan 5 21:57:35 2015 +0100

Make checkdoc happier on our docstrings

There is still work to do though.

commit 69066ce6503a12aa56a66f88ec3cfa462d57ae5e Author: Xavier Maillard xavier@maillard.im Date: Mon Jan 5 21:52:59 2015 +0100

Add support for customize UI in twittering-mode

1. Add 2 customize groups
2. Transform as much as possible defvar incatations to defcustom
3. Clean up trailing whitespaces (when it applies)

That's it. It should be pretty straight and safe to install.

But now I am stuck trying to incorporate them into the "latest version". Can you help ?

Regards