search

Fuco1 / smartparens

Minor mode for Emacs that deals with parens pairs and tries to be smart about it.
GNU General Public License v3.0
1.8k stars 193 forks source link

fix some of the tests; partially addresses #1201 #1202

Closed temyurchenko closed 2 months ago

Fuco1 commented 2 months ago

What is the new file that you created here for latex?

temyurchenko commented 2 months ago

What is the new file that you created here for latex?

The new file is for testing LaTeX-mode, as opposed to latex-mode. There is a lot of difference in how they handle electricity, so I felt it's worth it to test both of the modes.

The SaRcAsM-case names belong to the AUCTeX package, while the normal-case names belong to the builtin tex-mode package.

In particular, the seems to be a confusion about this in the sp--special-self-insert-commands variable. The comment says:

;; At some point the TeX and LaTeX functions were renamed to
;; lower-case names.  This broke some code dealing with these
;; modes, so we just add both versions for now.

But they've never been renamed, they just belong to different packages. E.g. TeX-insert-quote is from AUCTeX, and tex-insert-quote is from tex-mode.el. On the other hand, TeX-insert-dollar doesn't have a counterpart in the builtin tex-mode.el, so including it in the list seems excessive.

Fuco1 commented 2 months ago

Oh my, I never realized it's that stupid :D In that case yea it definitely makes sense to have tests for both modes, in fact ideally somehow "macroified" so we always test for both of them.

temyurchenko commented 2 months ago

I have considered the idea of «macroifiying» them. I have decided to go against that in this PR for the following reasons

  1. The modes are rather different. LaTeX-mode has a very different behaviour depending on the «electric» and other variables set and we might want to test against different combinations of those. They would be specific to each mode.
  2. Macros kind of tie you to a syntax that is at times hard to extend. For example, I initially wanted to mark all of the current test failures as «expected failures» (https://www.gnu.org/software/emacs/manual/html_mono/ert.html#Expected-Failures) [1]. However, due to heavy macro usage in sp-test sp-comment, we would have to extend macro syntax in some way.

So, given that they are kind of different, and macros are compromise, and that there are «only» two duplicates, I felt that the current solution is not the worst point of the design space.

[1]: Why? That way, we would still be able to run the test-suite regularly and track the regressions, whereas currently we might not notice that there is a new regression amidst those 42 failing tests. We would also have an issue tracking the progress of fixing the expected failures.

Fuco1 commented 2 months ago

[1]: Why? Then, there would be an issue tracking the progress of fixing the failures. That way, we would still be able to run the test-suite regularly and track the regressions, whereas currently we might not notice that there is a new regression amidst those 42 failing tests.

I didn't know it worked like that. I'm not really happy with how the tests are written, over many years we used many different approaches, some good and some bad, so yea, it's a huge mess now. It's one of my priorities now to get the test suite to pass everything, so I would rather focus on that since as you say the macro usage is quite heavy and makes things complicated.