GNUAspell / aspell

http://aspell.net
GNU Lesser General Public License v2.1
236 stars 50 forks source link

new TeX mode (again) #625

Closed matthias314 closed 2 months ago

matthias314 commented 1 year ago

This patch (hopefully) improves Aspell's TeX mode. It is practically identical to the one posted to the Aspell mailing list in 2011.

New features implemented by this patch

See the Info file for more details.

Comments about the changes

./modules/filter/context.cpp It seems that the order of the context filter was undefined. Whether one wants to have this filter before or after other filters depends on the application in mind. I need it before the tex filter so that one can place the aspell:off and aspell:on flags inside TeX comments.

./modules/filter/tex.cpp The new code for the tex filter.

./modules/filter/tex-filter.info The new default settings for the filter variables. tex-ignore-env is new. Most changes to tex-command are related to the new T option. The new begin line is used internally. The rest are additions and corrections to existing definitions.

./modules/filter/modes/tex.amf Note that we enable the context filter by default.

./manual/aspell.1 ./manual/aspell.texi Updated documentation.

dkwo commented 1 year ago

Thanks for your work. I'm unable to apply this patch cleanly on 0.60.8 Is there something I'm missing?

matthias314 commented 1 year ago

@dkwo: There have been several (more or less) recent commits fixing typos in the documentation, the latest being #624. They can be ignored. Are there any conflicts regarding the code?

dkwo commented 1 year ago

Applying the file obtained by appending .patch to this PR on top of the 0.60.8 tarball, besides the manual

1 out of 1 hunk FAILED -- saving rejects to file manual/aspell.1.rej 1 out of 5 hunks FAILED -- saving rejects to file manual/aspell.texi.rej

I get this failure:

3 out of 6 hunks FAILED -- saving rejects to file modules/filter/tex.cpp.rej

matthias314 commented 1 year ago

@dkwo: That's strange. I've downloaded the patch and the 0.60.8 tar ball. Then

/tmp/aspell-0.60.8$ patch --dry-run --verbose -p1 </tmp/625.patch

gives (among other things)

checking file modules/filter/tex.cpp
Using Plan A...
Hunk #1 succeeded at 32.
Hunk #2 succeeded at 53.
Hunk #3 succeeded at 71.
Hunk #4 succeeded at 99.
Hunk #5 succeeded at 267.
Hunk #6 succeeded at 294.
dkwo commented 1 year ago

My bad, sorry about that. I downloaded the patch again, and now it applies cleanly, if I remove the manuals part. It seems to work fine here, but I have one question. Suppose I want to ignore the \cref command (similar to \eqref and \ref): what is the correct synax to pass aspell?

matthias314 commented 1 year ago

To ignore \cref{label}, you put

add-tex-command cref p

into your .aspell.conf file. One could also add a corresponding definition to modules/filter/tex-filter.info, and for many other macros as well.

dkwo commented 1 year ago

Thank you.

dkwo commented 1 year ago

I've been using this for almost a year, can confirm it works well. @kevina any chance it can be merged?

kevina commented 7 months ago

@dkwo @matthias314 my main concern with this is causing a regression. I would be more willing to accept this if it has some test cases. There is now a primitive framework for this is the tests/ directory (see the filter-test target in the Makefile). Ideally I would like to first accept a pull request with some tests for the existing TeX filter to make sure this new filter doesn't break anything.

kevina commented 2 months ago

@matthias314, I am leaning towards accepting this once the above issues are resolved.

kevina commented 2 months ago

To be clear by above issues I mean the two comments in the code review, tests would be nice, but not really required.

matthias314 commented 2 months ago

I'm very sorry for my belated reply.

Regarding a potential regression: That's a valid point. I've been using the new TeX mode for many years now, but only on a very limited variety of LaTeX documents. By the same token, I think that if I got around to add test documents, then they would only cover a tiny fraction of the kinds of documents other users have. Would be a possible alternative to add the new TeX mode besides the existing one so that users could fall back to the old version in case the new one causes problems?

kevina commented 2 months ago

Would be a possible alternative to add the new TeX mode besides the existing one so that users could fall back to the old version in case the new one causes problems?

I thought about that, but rejected the idea as being too complicated, but that was many years ago. I'll have another look sometime soonish. Hopefully by the end of the weekend.

kevina commented 2 months ago

Closing in favor of #644