Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

key_without_language() reimplemented with regexp #136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atilante
Copy link
Contributor

@atilante atilante commented Dec 17, 2021

Description

What?

This pull request contains several related modifications to A+ RST Tools issue #134.
#134

  1. In lib/toc_languages.py, functions key_without_language() and join_keys()
    were refactored into methods in the class IndexJoiner. This is more consistent,
    as there are other "join"-named methods in IndexJoiner.

  2. Method 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.

  3. Method join_keys in lib/toc_languages.py now prints a warning message
    into log. The message describes the location of mismatching keys in the
    documentation tree, and mismatching keys with their language identifiers.
    Example:

    Mismatching keys at modules.1.2.3:
    Language: en Key: sorting_en_something
    Language: fi Key: sorting_fi_different

This should help the course material author locate the error. Moreover,
as the method now logs an error message instead of raising a SphinxError,
the compilation process can show log messages of all mismatching keys at
one compilation run, thus increasing productivity.

  1. 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.

Why?

A+ RST Tools could not produce a helpful error message when there was
something inconsistent with keys on multilingual courses.

How?

Described above.

Fixes #134

Testing

Python unit tests are introduced to A+ RST Tools in this pull request.
There is the new directory "tests".

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

[ADD A DESCRIPTION ABOUT WHAT YOU TESTED MANUALLY]

Did you test the changes in

  • Chrome
  • Firefox
  • This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

  • Did you modify or add new strings in the user interface?
    Original error message:
    Corresponding RST file names must match in multilingual courses:
    fi_something
    en_different
    
    Modified error message:
    Mismatching keys at modules.1.2.3:
    Language: en Key: sorting_en_something
    Language: fi Key: sorting_fi_different
    

Programming style

Have you updated the README or other relevant documentation?

  • documents inside the doc directory.
  • README.md.
  • Aplus Manual.
  • Other documentation: tests/README.md

Is it Done?

  • Reviewer has finished the code review
  • After the review, the developer has made changes accordingly
  • Customer/Teacher has accepted the implementation of the feature

Clean up your git commit history before submitting the pull request!

This commit contains several related modifications to A+ RST Tools issue apluslms#134.
apluslms#134

1. In lib/toc_languages.py, functions key_without_language() and join_keys()
were refactored into methods in the class IndexJoiner. This is more consistent,
as there are other "join"-named methods in IndexJoiner.

2. Method 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.

3. Method join_keys in lib/toc_languages.py now prints a warning message
into log. The message describes the location of mismatching keys in the
documentation tree, and mismatching keys with their language identifiers.
Example:

    Mismatching keys at modules.1.2.3:
    Language: en Key: sorting_en_something
    Language: fi Key: sorting_fi_different

This should help the course material author locate the error. Moreover,
as the method now logs an error message instead of raising a SphinxError,
the compilation process can show log messages of all mismatching keys at
one compilation run, thus increasing productivity.

4. 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.
@markkuriekkinen markkuriekkinen self-requested a review May 7, 2022 17:20
@markkuriekkinen markkuriekkinen self-assigned this May 7, 2022
@markkuriekkinen markkuriekkinen added type: bug This is a bug which is critical for the correct operation of the system area: UX teacher User experience and usability for teachers effort: hours The issue seems to be small and should be doable in hours or few days. experience: moderate required knowledge estimate requester: Aalto teacher The issue is raised by a teacher from Aalto University labels May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UX teacher User experience and usability for teachers effort: hours The issue seems to be small and should be doable in hours or few days. experience: moderate required knowledge estimate requester: Aalto teacher The issue is raised by a teacher from Aalto University type: bug This is a bug which is critical for the correct operation of the system
Projects
Status: Todo
2 participants