search

apluslms / a-plus-rst-tools

Tools to publish RST course content for mooc-grader and a-plus. Should be cloned as a course submodule.
MIT License
6 stars 24 forks source link

Inconsistent keys on multilingual course cause a cryptic error: "Corresponding RST file names must match in multilingual courses" #134

Open atilante opened 2 years ago

atilante commented 2 years ago

Description

If a multilingual course has an exercise or other item that has different identifier (key) in different language versions, A+ RST Tools produces a compile error which does not help to solve the problem.

Proposed categories: bug, user interface.

Note: I have a bugfix which could be sent as a pull request.

How to reproduce

Clone the Data Structures and Algorithms Y course repository with the try to compile the RST material. Exact commands:

git clone git@version.aalto.fi:course/traky.git -b rst-tools-issue-corresponding-names traky-test
cd traky-test
git submodule init
git submodule update
./docker-compile.sh exam

The output of docker-compile.sh script:

CS-A1141 + CS-A1143 exam selected
Compiling course.

--- lines cut ---

build succeeded, 2 warnings.

The HTML pages are in _build/html.
Detected language tree.
Traverse document elements to write configuration index (fi).
Traverse document elements to write configuration index (en).
Joining language tree to one index.

Sphinx error:
Corresponding RST file names must match in multilingual courses:
fi_enrollment
en_enrollment
make: *** [Makefile:62: html] Error 2

Why this is a problem

A+ RST Tools produces the error message: Corresponding RST file names must match in multilingual courses. While this is understandable, the lines below the message are rather cryptic:

fi_enrollment
en_enrollment

The user (a course author) should know on which files and which lines in the RST source code the error is located. Here A+ RST Tools dumps some internal string representation of some object, and we cannot know whether it is a file name or something inside an RST file.

In this particular case, the user is left to searching for the error location:

Investigation

The code location that produces the error message is https://github.com/apluslms/a-plus-rst-tools/blob/master/lib/toc_languages.py#L316 .

Proposal

I will send a bugfix as a pull request when you have time to review it.

markkuriekkinen commented 2 years ago

If I understood correctly, you are going to change only the error message. That's great! Please submit the pull request.

atilante commented 2 years ago

My pull request would actually contain three things. Here's the commit message summarising those. Issue #134 is number 2 in the following list.

1. Function key_without_language in lib/toc_languages.py had a bug: the
function could only strip language ids of form "id_" and "id-", not "_id" or
"_id_". The bug was discovered with course the CS-A1141 Data Structures and
Algorithms Y.  The function is now reimplemented with a more flexible regular
expression.

2. Function join_keys in lib/toc_languages.py now prints the original
keys in the case they could not be matched. Printing the original key allows
the course material author locate the point of error compared to the stripped
version of the key, which is likely shorter and thus a "needle in a haystack".

3. Unit test file tests/test_toc_languages.py tests the aforementioned
modified functions. The unit tests allow the A-plus RST tools developer to:
(i) easily test the functions of the software without needing to run a
    Sphinx compilation of a test material, which can take minutes;
(ii) document how the functions should respond to certain inputs.

The pull request contains 140 lines, including documentation and unit tests. As this is not a supercritical bugfix, would you like to review it in January rather than before the holidays?

markkuriekkinen commented 2 years ago

That's OK! Since you have already implemented it, you should submit the pull request. I will take a look at it when I'm able, either in December or January. I don't see why you should wait with making the PR. It will be there waiting for me and I can dig in immediately when I have the time.