rougier / nano-emacs

GNU Emacs / N Λ N O - Emacs made simple
GNU General Public License v3.0
2.52k stars 195 forks source link

Proposed refactor in order to make modules loadable independently #9

Closed mrzor closed 3 years ago

mrzor commented 3 years ago

Greetings!

Thanks for grouping all this goodness into one package! I've been following your works on Reddit for months now, and only in this incarnation I can reasonably hope to integrate some of your design ideas into my prelude-based config.

My focus last night was on integrating your "modeline" (header line, to be honest), and I managed to do so but I had to make quite a number of edits to nano-theme (to preserve my theme, that I'm very used to for now) and nano-modeline (which got completely mangled by parinfer on pasting, and that I had to debug to make work again).

Here's a sneak peak of what I got: image Brilliant. I hated my modeline for years. This is much, much better.

My issue is that I'm not happy to have forked such an early piece of code (that is going to change a lot) - and maintaining that fork is going to be a burden. I would much prefer to have nano-emacs code wholesale somewhere in my load path, and use what I want.

To that effect, I'm ready to contribute changes that would refactor the existing code into:

I started moving the pieces around, but I'm opening this issue with the hope of starting a conversation about modularity and code style before submitting it.

rougier commented 3 years ago

Thank for the feedback and what you proposed makes sense actually. At least for point 1 (we could have a nano-faces.el) and point 2 (code will be more consistent with the way I wrote nano-modeline.el). For the third point, I'm not sure to understand why a non conventional indentation would break anything since we have parens to delimit blocks. I would understand for Python but for elisp, I really wonder.

mrzor commented 3 years ago

I understand that you will see changes for points 1 and 2 favorably - as such I will keep doing my edits and I hope to be able to show a PR before tomorrow, to keep the discussion going on these points.

Regarding point 3, I understand this might sound confusing - elisp is indeed not white space sensitive. The glitch comes from parinfer-mode ; which infers parens from indentation ; and indentation from parens. It can get confused by unconventional indentation, which results in broken code.

Here's an example, when copying the whole nano-theme.el from a browser:

(defface nano-face-default nil
  "Default face is used for regular information.") ;; <- wrongly inferred parens here
:group 'nano

Where it should have worked better, like so:

(defface nano-face-default nil
  "Default face is used for regular information."
  :group 'nano) ;; <-- should have inferred white space at the beginning of this line

I concede that you could very well say "not my problem" and leave it there.

As it happens, fixing unconventional indentation in a few spots is a quick and easy workaround to this parinfer bug. Clean, conventional indentation makes the code subjectively easier to read. It hopefully provides a consensual code style that all contributors can try to stick to, which reduces review size in PR and makes git blame more useful.

rougier commented 3 years ago

In fact I do not like very much automated tools that think they're smarter than me in terms of indentation and I tend to favor readability over correctness, even it if confuses tools. However, I've not yet strong opinion about identation in elisp and I can try to stick as much as possible to the default scheme. Can you make a separate PR for the indentation? Also, is there some linters to check indendation? I tried parinfer-mode but I think we're not friends.

mrzor commented 3 years ago

I understand favoring readability. It is however a more subjective metric than correctness is.

With regards with linters, I casually looked into it yesterday and I have not found anything widely adopted / popular that would be a good first choice. There are a number of automatic code formatters - but they're

It's non trivial for me to isolate the indentation change. Wrapping all the face setting looks like a full file rewrite from a git perspective at this point. I tried not to reindent thing for reindenting sake but I did notice parinfer doing a few things. I'll try to fight it back with settings in further commits following your review.

rougier commented 3 years ago

You mean that asking emacs to re-indent the whole file would not do the trick ?

s-kostyaev commented 3 years ago

Also, is there some linters to check indendation? I tried parinfer-mode but I think we're not friends.

You can try aggressive-indent for elisp to prevent wrong indentation in the future.

mrzor commented 3 years ago

Using emacs itself to indent emacs lisp feels like the right way to go about it. I'm not sure how to have every contributor indentation settings converge though. First thing that comes to mind is having extensive # -*- ... -*- blocks on each file specifying file local indentation settings. Another way would be nano-codestyle.el that I can just require / call when needed - at the risk of forgetting doing it.

Code formatting tools tend to have their own isolated settings that can be versioned, which solves the problem of having identical indentation settings for everyone.

aggressive-indent looks interesting, but it's the polar opposite of parinfer : with parinfer, you manually indent and parens are edited appropriately, with aggressive-indent you manually edit parens and indent is edited appropriately. I don't believe one can have both modes enabled at once.

s-kostyaev commented 3 years ago

I don't believe one can have both modes enabled at once.

Sure.

s-kostyaev commented 3 years ago

you manually edit parens

I use electric-pair-mode for it. I prefer it over paredit-mode, because it's available out of the box and feels not so pedantic (I can feel control).

mrzor commented 3 years ago

The relevant PR was merged, so I think this issue should be closed.

The conversation about coding style and linting will probably resume at some point in the future. I have no consensual solution to propose on this question at this time.