Skip to content
This repository has been archived by the owner on Aug 14, 2022. It is now read-only.

Show Name of Undefined Control Sequence (macro) #496

Closed
wants to merge 22 commits into from

Conversation

Strauman
Copy link

When LaTeX complains about undefined control sequence, this plugin does
not show the name of said macro, which is what this commit attempts to
fix.

When LaTeX complains about undefined control sequence, this plugin does
not show the name of said macro, which is what this commit attempts to
fix.
@Strauman
Copy link
Author

Simple LaTeX test case:

\documentclass{minimal}
\def\once{Once}
\begin{document}
  \once upon a time there was an \undefined control sequence.
\end{document}

Expected log output:
screen shot 2018-11-11 at 11 52 27

// Pattern for obtaining the last macro in a line
// Meant for catching the undefined control sequence
const LAST_MACRO_PATTERN = new RegExp('' +
'(\\\\[^\\\\]+)$' // Macro name
Copy link
Author

@Strauman Strauman Nov 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not highly confident as to what ideally should a legal macro name in
this setting.

The problem is that the escape character, \ by default, can be redefined, as well
as having anything being active letters (and thus treated as control sequences)

E.g. doing

\catcode`O=13\relax O

would make the letter O an unknown control sequence, and

\catcode`J=0\relax JUndefined

would make JUnedfined behave identically to \Undefined and this atom plugin
will report weird information.

You can't just look at the last space and go from there. If you consider

{}\some@undef@macro

Then the {} would be a part of the macro name reported by atom-latex.

However, I do argue that these cases are extraordinary circumstances,
and isn't really that necessary to implement.

Copy link
Owner

@thomasjo thomasjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would really love it if we had a test covering this. The easiest way would probably be to use one of the log files in the fixtures folder (e.g. file.log) and augment it to contain an undefined control sequence error. Alternatively create a file which produces that error, and alter the locally generated log file by censoring local file paths, etc, like we've done for existing fixtures.

Having a test covering at least the "happy path" of this new logic ensures correct behavior, guards against regressions, and what not. Having tests that cover cases where a macro is not found would be amazing, but not strictly required.

lib/parsers/log-parser.js Outdated Show resolved Hide resolved
lib/parsers/log-parser.js Outdated Show resolved Hide resolved
lib/parsers/log-parser.js Outdated Show resolved Hide resolved
lib/parsers/log-parser.js Outdated Show resolved Hide resolved
lib/parsers/log-parser.js Outdated Show resolved Hide resolved
spec/process-manager-spec.js Outdated Show resolved Hide resolved
@Strauman
Copy link
Author

Strauman commented Mar 5, 2019

Having tests that cover cases where a macro is not found would be amazing, but not strictly required.

I'll see if I get the time.

Due to continuously timing out the test on this, I added a longer timeout.
This is now removed per request:
thomasjo#496 (comment)
- const where const is due
- Better variable names
- More canonical inline variables in string

Also a few extra comments
As per review request, now checking that there exists at least one line
below "Unknown control sequence" error before starting to parse it.
@thomasjo thomasjo closed this Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants