Added support for producing latex code #13
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (10)
static/en/mystery.template.tex (2)
7-9: Improve image handling robustness.Consider these improvements for better image handling:
- Specify the file extension (e.g.,
locations_small.png) or use multiple extensions for fallback- Add error handling for missing images using
\IfFileExists- \includegraphics[height=0.15\paperheight]{locations_small} + \IfFileExists{locations_small.png}{% + \includegraphics[height=0.15\paperheight]{locations_small.png}% + }{% + \textbf{[Location image not available]}% + }
11-19: Document table generation requirements.The placeholders
$$cluesTablesand$$weaponsTablesuggest dynamic table generation. Consider adding comments to document:
- Expected table structure and format
- Required column headers
- Any specific LaTeX table packages needed (e.g.,
tabularx,longtable)static/es/mystery.template.tex (3)
1-10: Improve image handling and spacing in the introduction section.Consider the following improvements:
- Add error handling for the image using
\IfFileExists- Consider using
\vspacefor consistent vertical spacing- Use a more robust approach for image scaling
\subsection*{\emoji{robot} beep boop, hay un nuevo misterio} +\vspace{0.5em} $$introLocation +\vspace{0.3em} Ayúdales a resolver este misterio revisando estas pistas iniciales: $$initialClues +\vspace{1em} \begin{center} - \includegraphics[height=0.15\paperheight]{locations_small} + \IfFileExists{locations_small}{% + \includegraphics[width=0.8\linewidth,height=0.15\paperheight,keepaspectratio]{locations_small} + }{% + \textbf{[Image 'locations\_small' not found]} + } \par\end{center}
11-19: Optimize layout in detective's diary section.The section could benefit from better spacing and structure:
- Remove redundant
\noindentbefore\begin{center}- Add vertical spacing between tables for better readability
\subsubsection*{\emoji{notebook} Tu diario de detective y tus pistas} -\noindent \begin{center} +\begin{center} $$cluesTables +\vspace{1em} $$weaponsTable +\vspace{1em} \par\end{center} $$additionalClues
1-31: Add documentation and improve accessibility.Consider the following improvements for better maintainability and accessibility:
- Add comments explaining the expected format and content of each placeholder ($$introLocation, $$initialClues, etc.)
- Consider adding screen reader support using
\pdftooltipor similar commands for emojis- Consider adding PDF bookmarks for better navigation
Would you like me to provide examples of these improvements?
setup.py (1)
33-33: Consider pinning the emoji package version.While the addition of the emoji package is justified for LaTeX output, consider pinning its version to ensure consistent behavior across different environments.
- "emoji", + "emoji==2.8.0", # or another stable versionmystery_o_matic/output/latex/utils.py (3)
17-30: Refactor list generation to reduce code duplication.The functions have similar structure and contain an unused
nameparameter.-def get_bullet_list(elements, name=""): +def get_latex_list(elements, list_type="itemize"): + """Generate a LaTeX list from elements. + + Args: + elements: List of elements to include + list_type: Either "itemize" or "enumerate" + """ r = "\\begin{" + list_type + "}\n" for element in elements: r += "\\item " + element + "\n" - r += "\\end{itemize}" + r += "\\end{" + list_type + "}" return r -def get_enumeration_list(elements, name=""): - r = "\\begin{enumerate}\n" - for element in elements: - r += "\\item " + element + "\n" - r += "\\end{enumerate}" - return r +def get_bullet_list(elements): + return get_latex_list(elements, "itemize") + +def get_enumeration_list(elements): + return get_latex_list(elements, "enumerate")
55-59: Use regex for more robust emoji replacement.The current string replacement approach could be fragile when handling nested parentheses or colons in the text.
+import re + def replace_emojis(text): - no_emojis = demojize(text).replace("(:", "(\\emoji{") - no_emojis = no_emojis.replace(":)", "})") - no_emojis = no_emojis.replace("_", "-") + # Replace emoji patterns like (:<name>:) with (\emoji{<name>}) + no_emojis = demojize(text) + no_emojis = re.sub(r'\(:([^:]+):\)', r'(\\emoji{\1})', no_emojis) + no_emojis = no_emojis.replace("_", "-") return no_emojis
61-101: Add documentation and break down complex table generation.The function would benefit from:
- Comprehensive documentation
- Breaking down into smaller, focused functions
- Configuration parameters for column widths
Consider extracting the header and row generation into separate functions and adding proper documentation:
def generate_table_header(num_columns): """Generate the header row for the clue table.""" header = "& \\emoji{mantelpiece-clock} & " time_cells = [f"$$TIME{i}" for i in range(num_columns)] return header + " & ".join(time_cells) + " \\tabularnewline\n" def generate_clines(start_col, end_col): """Generate cline commands for the specified column range.""" return "".join([f"\\cline{{{col}-{end_col}}} " for col in range(start_col, end_col + 1)])This would make the main function more readable and maintainable.
mystery_o_matic/__main__.py (1)
113-116: Update error message to include LaTeX mode.The error message should be updated to reflect all supported modes.
- print("Only html and text is accepted") + print("Only html, text, and latex modes are accepted")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/ci.yaml(0 hunks)mystery_o_matic/__main__.py(3 hunks)mystery_o_matic/output/__init__.py(0 hunks)mystery_o_matic/output/latex/__init__.py(1 hunks)mystery_o_matic/output/latex/utils.py(1 hunks)setup.py(2 hunks)static/en/mystery.template.tex(1 hunks)static/es/mystery.template.tex(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/ci.yaml
- mystery_o_matic/output/init.py
🧰 Additional context used
🪛 Ruff
mystery_o_matic/output/latex/__init__.py
30-30: Ambiguous variable name: l
(E741)
31-31: Ambiguous variable name: l
(E741)
36-36: Ambiguous variable name: l
(E741)
37-37: Ambiguous variable name: l
(E741)
69-69: Loop control variable i not used within loop body
(B007)
97-97: Loop control variable i not used within loop body
(B007)
124-124: Loop control variable x not used within loop body
Rename unused x to _x
(B007)
130-130: Loop control variable char not used within loop body
(B007)
133-133: Loop control variable name not used within loop body
(B007)
139-139: Ambiguous variable name: l
(E741)
mystery_o_matic/output/latex/utils.py
32-35: Use contextlib.suppress(OSError) instead of try-except-pass
Replace with contextlib.suppress(OSError)
(SIM105)
🔇 Additional comments (5)
static/en/mystery.template.tex (1)
1-5: Verify emoji package dependency.
The template uses \emoji command for rendering emojis. Ensure that the required LaTeX package (likely emoji or similar) is properly included in the main document preamble.
setup.py (1)
20-20: LGTM: New LaTeX output package follows project structure.
The addition of mystery_o_matic.output.latex package is consistent with the existing output format structure and aligns with the PR objective.
mystery_o_matic/output/latex/utils.py (1)
1-8: LGTM! Good choice of custom delimiter.
The custom $$ delimiter is a smart choice as it avoids conflicts with LaTeX's $ math mode delimiter.
mystery_o_matic/__main__.py (2)
12-12: LGTM: Import statement follows project conventions.
The new LaTeX output module import aligns with the existing output module structure.
189-192: Consider extracting hardcoded language list.
The language list ['en', 'es'] is duplicated in both HTML and LaTeX output calls. Consider extracting it to a configuration constant or parameter.
+SUPPORTED_LANGUAGES = ['en', 'es']
+
if mode == "html":
produce_html_output(
- static_dir, out_dir, ['en', 'es'], mystery, weapons_available, weapon_labels, locations, story_clue
+ static_dir, out_dir, SUPPORTED_LANGUAGES, mystery, weapons_available, weapon_labels, locations, story_clue
)
elif mode == "latex":
produce_tex_output(
- static_dir, out_dir, ['en', 'es'], mystery, weapons_available, weapon_labels, locations, story_clue
+ static_dir, out_dir, SUPPORTED_LANGUAGES, mystery, weapons_available, weapon_labels, locations, story_clue
)Let's verify the LaTeX output implementation:
✅ Verification successful
Consider extracting hardcoded language list - Verified Implementation
The LaTeX output implementation exists and is properly structured:
- The
produce_tex_outputfunction is implemented inmystery_o_matic/output/latex/__init__.py - It has comprehensive LaTeX utilities in
mystery_o_matic/output/latex/utils.py - The function accepts the same parameters as suggested in the review
The suggestion to extract the hardcoded language list is valid as it appears in multiple output modes (HTML, LaTeX) and would improve maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the LaTeX output module is properly implemented
# Test: Check if the LaTeX output module exists and contains the required function
ast-grep --pattern 'def produce_tex_output($$$)'
Length of output: 50
Script:
#!/bin/bash
# Let's try a broader search to find any LaTeX-related functionality
rg -i "latex|tex" --type py -A 5 -B 5
Length of output: 19885
| \noindent\ovalbox{\begin{minipage}[t]{1\columnwidth - 2\fboxsep - 0.8pt}% | ||
| \medskip{} | ||
|
|
||
| \begin{center} | ||
| \begin{tabular}{ccccc} | ||
| The murderer was... & & The weapon was... & & Time of murder was...\tabularnewline | ||
| & & & & \tabularnewline | ||
| \cline{1-1} \cline{3-3} \cline{5-5} | ||
| & & & & \tabularnewline | ||
| \end{tabular} | ||
| \par\end{center}% | ||
| \end{minipage}} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the solution box layout and usability.
The current table structure has several potential issues:
- Fixed width calculation might not work well with different paper sizes
- Table cells lack sufficient space for writing answers
- Table alignment could be improved
-\noindent\ovalbox{\begin{minipage}[t]{1\columnwidth - 2\fboxsep - 0.8pt}%
+\noindent\ovalbox{\begin{minipage}[t]{\dimexpr\linewidth-2\fboxsep-2\fboxrule\relax}%
\medskip{}
\begin{center}
-\begin{tabular}{ccccc}
-The murderer was... & & The weapon was... & & Time of murder was...\tabularnewline
- & & & & \tabularnewline
-\cline{1-1} \cline{3-3} \cline{5-5}
- & & & & \tabularnewline
+\begin{tabular}{p{0.3\linewidth}@{\hspace{1em}}p{0.3\linewidth}@{\hspace{1em}}p{0.3\linewidth}}
+\textbf{The murderer was...} & \textbf{The weapon was...} & \textbf{Time of murder was...}\tabularnewline[1em]
+\cline{1-1}\cline{2-2}\cline{3-3}
+\rule{0pt}{3em} & \rule{0pt}{3em} & \rule{0pt}{3em}\tabularnewline
\end{tabular}
\par\end{center}%
\end{minipage}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| \noindent\ovalbox{\begin{minipage}[t]{1\columnwidth - 2\fboxsep - 0.8pt}% | |
| \medskip{} | |
| \begin{center} | |
| \begin{tabular}{ccccc} | |
| The murderer was... & & The weapon was... & & Time of murder was...\tabularnewline | |
| & & & & \tabularnewline | |
| \cline{1-1} \cline{3-3} \cline{5-5} | |
| & & & & \tabularnewline | |
| \end{tabular} | |
| \par\end{center}% | |
| \end{minipage}} | |
| \noindent\ovalbox{\begin{minipage}[t]{\dimexpr\linewidth-2\fboxsep-2\fboxrule\relax}% | |
| \medskip{} | |
| \begin{center} | |
| \begin{tabular}{p{0.3\linewidth}@{\hspace{1em}}p{0.3\linewidth}@{\hspace{1em}}p{0.3\linewidth}} | |
| \textbf{The murderer was...} & \textbf{The weapon was...} & \textbf{Time of murder was...}\tabularnewline[1em] | |
| \cline{1-1}\cline{2-2}\cline{3-3} | |
| \rule{0pt}{3em} & \rule{0pt}{3em} & \rule{0pt}{3em}\tabularnewline | |
| \end{tabular} | |
| \par\end{center}% | |
| \end{minipage}} |
| for l, r in locations.representations.items(): | ||
| l = create_template("$" + l).substitute(names_txt) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename ambiguous variable l to improve code readability
Using single-character variable names like l can be confusing and reduce code readability, as they can be mistaken for 1 or I. It's recommended to use more descriptive variable names.
Apply this diff to rename l to location_key:
-for l, r in locations.representations.items():
- l = create_template("$" + l).substitute(names_txt)
- representations_map[l] = r
+for location_key, r in locations.representations.items():
+ location_substituted = create_template("$" + location_key).substitute(names_txt)
+ representations_map[location_substituted] = r📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for l, r in locations.representations.items(): | |
| l = create_template("$" + l).substitute(names_txt) | |
| for location_key, r in locations.representations.items(): | |
| location_substituted = create_template("$" + location_key).substitute(names_txt) | |
| representations_map[location_substituted] = r |
🧰 Tools
🪛 Ruff
30-30: Ambiguous variable name: l
(E741)
31-31: Ambiguous variable name: l
(E741)
There was a problem hiding this comment.
This kind of issue will be improved in a future PR. Please create an issue about this and resolve this conversation.
There was a problem hiding this comment.
@neuromancer, I've created Issue #16 to track this improvement and will resolve this conversation.
🔗 Analysis chain
@neuromancer, I've created an issue to track this improvement and will resolve this conversation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a new GitHub issue about refactoring ambiguous variable names for better readability
gh issue create \
--title "Refactor ambiguous variable names for better readability" \
--body "In PR #13 ([https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13](https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13)), it was suggested to rename ambiguous variable names like \`l\` to more descriptive names to improve code readability.
Referenced comment: [https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13#discussion_r1832200713](https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13#discussion_r1832200713)
Requested by @neuromancer." \
--assignee neuromancer
Length of output: 693
| for l, w in locations.weapon_locations.items(): | ||
| l = create_template("$" + l).substitute(names_txt) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename ambiguous variable l to improve code readability
Again, using l as a variable name can be confusing. Consider renaming it to location_key for clarity.
Apply this diff:
-for l, w in locations.weapon_locations.items():
- l = create_template("$" + l).substitute(names_txt)
- weapons_map[w] = l
+for location_key, w in locations.weapon_locations.items():
+ location_substituted = create_template("$" + location_key).substitute(names_txt)
+ weapons_map[w] = location_substituted📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for l, w in locations.weapon_locations.items(): | |
| l = create_template("$" + l).substitute(names_txt) | |
| for location_key, w in locations.weapon_locations.items(): | |
| location_substituted = create_template("$" + location_key).substitute(names_txt) | |
| weapons_map[w] = location_substituted |
🧰 Tools
🪛 Ruff
36-36: Ambiguous variable name: l
(E741)
37-37: Ambiguous variable name: l
(E741)
| for l, w in locations.weapon_locations.items(): | ||
| args[l + "WEAPONREP"] = get_emoji_name(locations.representations[l]) + " " + get_emoji_name(weapons[w]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename ambiguous variable l to improve code readability
Using l as a variable name can reduce readability. Consider renaming it to location_key.
Apply this diff:
-for l, w in locations.weapon_locations.items():
- args[l + "WEAPONREP"] = get_emoji_name(locations.representations[l]) + " " + get_emoji_name(weapons[w])
+for location_key, w in locations.weapon_locations.items():
+ args[location_key + "WEAPONREP"] = get_emoji_name(locations.representations[location_key]) + " " + get_emoji_name(weapons[w])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for l, w in locations.weapon_locations.items(): | |
| args[l + "WEAPONREP"] = get_emoji_name(locations.representations[l]) + " " + get_emoji_name(weapons[w]) | |
| for location_key, w in locations.weapon_locations.items(): | |
| args[location_key + "WEAPONREP"] = get_emoji_name(locations.representations[location_key]) + " " + get_emoji_name(weapons[w]) |
🧰 Tools
🪛 Ruff
139-139: Ambiguous variable name: l
(E741)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
mystery_o_matic/output/latex/utils.py (2)
69-109: Add docstring and make table dimensions configurable.The function lacks documentation and uses magic numbers for column widths.
def generate_latex_clue_table(name, num_columns, num_rows, include_header = True): + """Generate a LaTeX table for clues. + + Args: + name: The name to display in the first column + num_columns: Number of time slots + num_rows: Number of character rows + include_header: Whether to include the time header row + + Returns: + str: LaTeX code for the clue table + """ + # Column width as a fraction of paper width + COLUMN_WIDTH = 0.04 # Define the LaTeX column specifier string - column_spec = '|c|c|' + '|'.join(['>{\\centering}m{0.04\\paperwidth}' for _ in range(num_columns)]) + '|' + column_spec = '|c|c|' + '|'.join([f'>{{\\centering}}m{{{COLUMN_WIDTH}\\paperwidth}}' for _ in range(num_columns)]) + '|'
110-118: Add docstring and make weapons table more flexible.The function lacks documentation and uses hardcoded column widths.
-def generate_latex_weapons_table(number_weapons): +def generate_latex_weapons_table(number_weapons, column_width=0.172): + """Generate a LaTeX table for weapons. + + Args: + number_weapons: Number of weapons to include + column_width: Width of each column as a fraction of paper width + + Returns: + str: LaTeX code for the weapons table + """ - latex_code = '\\begin{tabular}{|>{\centering}p{0.172\paperwidth}|>{\centering}p{0.172\paperwidth}|>{\centering}p{0.172\paperwidth}|>{\centering}p{0.172\paperwidth}|}\n' + column_spec = '|' + '|'.join([f'>{{\\centering}}p{{{column_width}\\paperwidth}}' for _ in range(number_weapons)]) + '|' + latex_code = f'\\begin{{tabular}}{{{column_spec}}}\n'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
mystery_o_matic/output/latex/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff
mystery_o_matic/output/latex/utils.py
19-19: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
21-21: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| def get_bullet_list(elements, name=""): | ||
| r = "\\begin{itemize}\n" | ||
| for element in elements: | ||
| r += "\\item " + element + "\n" | ||
| r += "\\end{itemize}" | ||
| return r | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused parameter and optimize string operations.
Issues found:
- The
nameparameter is not used in either function - String concatenation in loops is inefficient
Apply this optimization to both functions:
-def get_bullet_list(elements, name=""):
+def get_bullet_list(elements):
- r = "\\begin{itemize}\n"
- for element in elements:
- r += "\\item " + element + "\n"
- r += "\\end{itemize}"
- return r
+ items = ["\\item " + element for element in elements]
+ return "\\begin{itemize}\n" + "\n".join(items) + "\n\\end{itemize}"
-def get_enumeration_list(elements, name=""):
+def get_enumeration_list(elements):
- r = "\\begin{enumerate}\n"
- for element in elements:
- r += "\\item " + element + "\n"
- r += "\\end{enumerate}"
- return r
+ items = ["\\item " + element for element in elements]
+ return "\\begin{enumerate}\n" + "\n".join(items) + "\n\\end{enumerate}"Also applies to: 30-36
| def replace_emojis(text): | ||
| no_emojis = demojize(text).replace("(:", "(\\emoji{") | ||
| no_emojis = no_emojis.replace(":)", "})") | ||
| no_emojis = no_emojis.replace("_", "-") | ||
| return no_emojis |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make emoji replacement more robust.
The current implementation using string replacement is fragile and could accidentally replace non-emoji text. Consider using regex or the emoji library's functionality directly.
def replace_emojis(text):
- no_emojis = demojize(text).replace("(:", "(\\emoji{")
- no_emojis = no_emojis.replace(":)", "})")
- no_emojis = no_emojis.replace("_", "-")
- return no_emojis
+ import re
+ # Convert emojis to their names
+ text_with_names = demojize(text)
+ # Replace emoji names with LaTeX commands
+ pattern = r':([\w-]+):'
+ return re.sub(pattern, r'\\emoji{\1}', text_with_names).replace("_", "-")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def replace_emojis(text): | |
| no_emojis = demojize(text).replace("(:", "(\\emoji{") | |
| no_emojis = no_emojis.replace(":)", "})") | |
| no_emojis = no_emojis.replace("_", "-") | |
| return no_emojis | |
| def replace_emojis(text): | |
| import re | |
| # Convert emojis to their names | |
| text_with_names = demojize(text) | |
| # Replace emoji names with LaTeX commands | |
| pattern = r':([\w-]+):' | |
| return re.sub(pattern, r'\\emoji{\1}', text_with_names).replace("_", "-") |
| def save_tex(outdir, language, tex): | ||
| output_dir = f"{outdir}/{language}" | ||
| with suppress(OSError): | ||
| makedirs(output_dir) | ||
|
|
||
| filename = f"{output_dir}/mystery.tex" | ||
| try: | ||
| with open(filename, "w", encoding='utf-8') as f: | ||
| f.write(tex) | ||
| except OSError as e: | ||
| raise RuntimeError(f"Failed to save LaTeX file: {e}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Chain exceptions in error handling.
Improve error handling by preserving the original exception traceback.
try:
with open(filename, "w", encoding='utf-8') as f:
f.write(tex)
except OSError as e:
- raise RuntimeError(f"Failed to save LaTeX file: {e}")
+ raise RuntimeError(f"Failed to save LaTeX file: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def save_tex(outdir, language, tex): | |
| output_dir = f"{outdir}/{language}" | |
| with suppress(OSError): | |
| makedirs(output_dir) | |
| filename = f"{output_dir}/mystery.tex" | |
| try: | |
| with open(filename, "w", encoding='utf-8') as f: | |
| f.write(tex) | |
| except OSError as e: | |
| raise RuntimeError(f"Failed to save LaTeX file: {e}") | |
| def save_tex(outdir, language, tex): | |
| output_dir = f"{outdir}/{language}" | |
| with suppress(OSError): | |
| makedirs(output_dir) | |
| filename = f"{output_dir}/mystery.tex" | |
| try: | |
| with open(filename, "w", encoding='utf-8') as f: | |
| f.write(tex) | |
| except OSError as e: | |
| raise RuntimeError(f"Failed to save LaTeX file: {e}") from e |
🧰 Tools
🪛 Ruff
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
This will be solved later. Please add a comment about it in #15
There was a problem hiding this comment.
@neuromancer, I've added a comment to issue #15 as requested.
🔗 Analysis chain
@neuromancer, I've added a comment to issue #15 as requested.
Apologies for the error in the script. The backticks around save_tex() were causing the syntax issue. Here's the corrected script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Add a comment to issue #15 noting that the exception chaining will be addressed there.
gh issue comment 15 --body "$(cat <<EOF
Noted that the exception chaining in the `save_tex()` function will be addressed in this issue, as per [PR #13](https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13) and [this discussion](https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13#discussion_r1833874647).
EOF
)"
Length of output: 525
Script:
#!/bin/bash
# Add a comment to issue #15 noting that the exception chaining will be addressed there.
gh issue comment 15 --body "$(cat <<'EOF'
Noted that the exception chaining in the `save_tex()` function will be addressed in this issue, as per [PR #13](https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13) and [this discussion](https://github.com/mystery-o-matic/mystery-o-matic.github.io/pull/13#discussion_r1833874647).
EOF
)"
Length of output: 430
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
mystery_o_matic/output/latex/__init__.py (8)
9-21: Refactor duplicate character enumeration logic.The character enumeration logic is duplicated for HTML and text formats. Consider extracting this into a helper function.
Apply this diff:
+def process_character_names(characters, format_func): + """Process character names into the specified format. + + Args: + characters: List of character names + format_func: Function to format each character name + + Returns: + Dictionary mapping character indices to formatted names + """ + return { + f"CHAR{i + 1}": format_func(char) + for i, char in enumerate(characters) + } + names_html = {} -for i, char in enumerate(mystery.get_characters()): - names_html["CHAR" + str(i + 1)] = get_char_name(char) +names_html.update(process_character_names( + mystery.get_characters(), + get_char_name +)) names_html["SUS0"] = suspects[0].capitalize() names_html["SUS1"] = suspects[1].capitalize() names_txt = {} -for i, char in enumerate(mystery.get_characters()): - names_txt["CHAR" + str(i + 1)] = char.lower() +names_txt.update(process_character_names( + mystery.get_characters(), + str.lower +))
40-49: Improve language handling with a more robust approach.The current implementation has hard-coded language support. Consider:
- Using an enum or constant for supported languages
- Providing a more informative error message
Apply this diff:
+from enum import Enum + +class SupportedLanguage(Enum): + ENGLISH = "en" + SPANISH = "es" + + @classmethod + def get_supported_languages(cls) -> str: + return ", ".join(lang.value for lang in cls) for language in languages: - if language == "en": + if language == SupportedLanguage.ENGLISH.value: names_html["NOBODY"] = "nobody" names_txt["NOBODY"] = "nobody" - elif language == "es": + elif language == SupportedLanguage.SPANISH.value: names_html["NOBODY"] = "nadie" names_txt["NOBODY"] = "nadie" else: - raise ValueError("Unknown language: " + language) + raise ValueError( + f"Unsupported language: {language}. " + f"Supported languages are: {SupportedLanguage.get_supported_languages()}" + )
51-53: Simplify character list joining using standard library.The character list joining can be simplified using
str.join().Apply this diff:
characters = list(map(lambda char: char.capitalize(), mystery.get_characters())) -introLocation = ", ".join(characters[:-1]) + " and " + characters[-1] +introLocation = f"{', '.join(characters[:-1])} and {characters[-1]}" introLocation += locations.intro[language]
55-65: Improve separation of concerns in label processing.The label processing logic mixes language-specific rules with general processing. Consider:
- Moving the skip condition to a filter
- Extracting language-specific formatting into a separate function
Apply this diff:
+def format_label_for_language(label: str, language: str) -> str: + """Format a label according to language-specific rules.""" + return label.capitalize() if language == "es" else label + +def is_valid_room(room: str, locations) -> bool: + """Check if a room should be included in processing.""" + return room in locations.rindices + -for room, name in locations.names[language].items(): - if room not in locations.rindices: - continue # skip any missing place +for room, name in filter( + lambda x: is_valid_room(x[0], locations), + locations.names[language].items() +): index = locations.rindices[room] names_html[index] = name + " (" + get_emoji_name(locations.representations[index]) + ")" for weapon, label in weapon_labels[language].items(): if weapon not in weapons: continue - if language == "es": - label = label.capitalize() + label = format_label_for_language(label, language) names_html[weapon.replace("$", "")] = label + " (" + get_emoji_name(weapons[weapon]) + ")"
67-67: Remove commented debug print statement.Debug print statements should be removed from production code.
Apply this diff:
-#print(names_html)
103-109: Remove commented out code.Commented out code should be removed if it's no longer needed. If this code might be needed in the future, consider tracking it in version control or documenting the requirement.
Apply this diff:
-#additional_clues_with_lies = [] - -#for i, clue in enumerate(mystery.additional_clues_with_lies): -# additional_clues_with_lies.append( -# create_template(clue[language]).substitute(names_html) -# )
111-128: Clean up commented code and extract format strings as constants.The code contains commented out code and uses hardcoded format strings.
Apply this diff:
+ROOM_FORMAT = "ROOM{}REP" + # populate the weapon options weapons_options = [] for w in locations.weapon_locations.values(): # use the labels in the current language but the values in english weapons_options.append((weapon_labels[language][w], weapon_labels["en"][w])) -#select_weapons = get_options_selector(weapons_options) args = {} args["introLocation"] = introLocation args["initialClues"] = initial_clues args["additionalClues"] = additional_clues_enumeration tables = "" for i in range(len(locations.weapon_locations.items())): - tables += generate_latex_clue_table(f"ROOM{i}REP", len(intervals), len(mystery.get_characters()), i == 0) + "\n" + tables += generate_latex_clue_table( + ROOM_FORMAT.format(i), + len(intervals), + len(mystery.get_characters()), + i == 0 + ) + "\n"
129-144: Optimize dictionary key iteration.The code unnecessarily uses
.keys()when iterating over dictionary keys.Apply this diff:
for i in range(len(mystery.get_characters())): args[f"CHAR{i+1}"] = names_html[f"CHAR{i+1}"] -for room in locations.names[language].keys(): +for room in locations.names[language]: if room not in locations.rindices: continue # skip any missing place index = locations.rindices[room] args[index + "REP"] = get_emoji_name(locations.representations[index])🧰 Tools
🪛 Ruff
133-133: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
139-139: Ambiguous variable name:
l(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
mystery_o_matic/output/latex/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff
mystery_o_matic/output/latex/__init__.py
30-30: Ambiguous variable name: l
(E741)
31-31: Ambiguous variable name: l
(E741)
36-36: Ambiguous variable name: l
(E741)
37-37: Ambiguous variable name: l
(E741)
69-69: Loop control variable i not used within loop body
(B007)
97-97: Loop control variable i not used within loop body
(B007)
133-133: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
139-139: Ambiguous variable name: l
(E741)
| from mystery_o_matic.output import create_template | ||
| from mystery_o_matic.output.latex.utils import generate_latex_clue_table, generate_latex_weapons_table, get_bullet_list, get_enumeration_list, read_tex_template, create_tex_template, save_tex, get_char_name, get_emoji_name, replace_emojis | ||
| from mystery_o_matic.clues import NoOneElseStatement | ||
|
|
||
| def produce_tex_output(static_dir, out_dir, languages, mystery, weapons, weapon_labels, locations, story_clue): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and docstring for better code maintainability.
The function signature would benefit from type hints and a comprehensive docstring explaining its purpose, parameters, and return value.
Apply this diff:
+from typing import List, Dict, Any
+
from mystery_o_matic.output import create_template
from mystery_o_matic.output.latex.utils import generate_latex_clue_table, generate_latex_weapons_table, get_bullet_list, get_enumeration_list, read_tex_template, create_tex_template, save_tex, get_char_name, get_emoji_name, replace_emojis
from mystery_o_matic.clues import NoOneElseStatement
-def produce_tex_output(static_dir, out_dir, languages, mystery, weapons, weapon_labels, locations, story_clue):
+def produce_tex_output(
+ static_dir: str,
+ out_dir: str,
+ languages: List[str],
+ mystery: Any, # Replace with actual Mystery type
+ weapons: Dict[str, str],
+ weapon_labels: Dict[str, Dict[str, str]],
+ locations: Any, # Replace with actual Locations type
+ story_clue: str
+) -> None:
+ """Generate LaTeX output for a mystery game.
+
+ Args:
+ static_dir: Directory containing static template files
+ out_dir: Directory where output files will be saved
+ languages: List of supported languages
+ mystery: Mystery game object containing game state
+ weapons: Dictionary mapping weapon IDs to their representations
+ weapon_labels: Dictionary mapping languages to weapon label dictionaries
+ locations: Object containing location information
+ story_clue: Story-related clue text
+
+ Raises:
+ ValueError: If an unsupported language is provided
+ """Committable suggestion skipped: line range outside the PR's diff.
| tex_template = read_tex_template(static_dir + f"/{language}/mystery.template.tex") | ||
| tex_source = tex_template.substitute(args) | ||
| tex_source = create_tex_template(tex_source).substitute(args) | ||
| save_tex(out_dir, language, tex_source) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and document double substitution.
The template processing lacks error handling for file operations, and the purpose of double substitution isn't clear.
Apply this diff:
+def process_template(static_dir: str, language: str, args: dict) -> str:
+ """Process the LaTeX template with arguments.
+
+ Performs a two-pass template substitution:
+ 1. First pass: Substitute the main template variables
+ 2. Second pass: Substitute any template variables in the substituted values
+
+ Args:
+ static_dir: Directory containing template files
+ language: Current language
+ args: Template arguments
+
+ Returns:
+ Processed template string
+
+ Raises:
+ FileNotFoundError: If template file doesn't exist
+ ValueError: If template substitution fails
+ """
+ template_path = f"{static_dir}/{language}/mystery.template.tex"
+ try:
+ tex_template = read_tex_template(template_path)
+ tex_source = tex_template.substitute(args)
+ return create_tex_template(tex_source).substitute(args)
+ except FileNotFoundError:
+ raise FileNotFoundError(f"Template file not found: {template_path}")
+ except ValueError as e:
+ raise ValueError(f"Template substitution failed: {str(e)}")
+
-tex_template = read_tex_template(static_dir + f"/{language}/mystery.template.tex")
-tex_source = tex_template.substitute(args)
-tex_source = create_tex_template(tex_source).substitute(args)
+tex_source = process_template(static_dir, language, args)
save_tex(out_dir, language, tex_source)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tex_template = read_tex_template(static_dir + f"/{language}/mystery.template.tex") | |
| tex_source = tex_template.substitute(args) | |
| tex_source = create_tex_template(tex_source).substitute(args) | |
| save_tex(out_dir, language, tex_source) | |
| def process_template(static_dir: str, language: str, args: dict) -> str: | |
| """Process the LaTeX template with arguments. | |
| Performs a two-pass template substitution: | |
| 1. First pass: Substitute the main template variables | |
| 2. Second pass: Substitute any template variables in the substituted values | |
| Args: | |
| static_dir: Directory containing template files | |
| language: Current language | |
| args: Template arguments | |
| Returns: | |
| Processed template string | |
| Raises: | |
| FileNotFoundError: If template file doesn't exist | |
| ValueError: If template substitution fails | |
| """ | |
| template_path = f"{static_dir}/{language}/mystery.template.tex" | |
| try: | |
| tex_template = read_tex_template(template_path) | |
| tex_source = tex_template.substitute(args) | |
| return create_tex_template(tex_source).substitute(args) | |
| except FileNotFoundError: | |
| raise FileNotFoundError(f"Template file not found: {template_path}") | |
| except ValueError as e: | |
| raise ValueError(f"Template substitution failed: {str(e)}") | |
| tex_source = process_template(static_dir, language, args) | |
| save_tex(out_dir, language, tex_source) |
| bullets = [] | ||
| for i, clue in enumerate(mystery.initial_clues): | ||
| clue = create_template(clue[language]).substitute(names_html) | ||
| bullets.append(clue) | ||
|
|
||
| sub_bullets = [] | ||
| for clue in mystery.weapon_locations_clues: | ||
| clue = create_template(clue[language]).substitute(names_html) | ||
| sub_bullets.append(clue) | ||
|
|
||
| weapon_locations_bullets = mystery.weapon_locations_intro[language] | ||
| weapon_locations_bullets += get_bullet_list(sub_bullets, language) | ||
| bullets.append(weapon_locations_bullets) | ||
| bullets.append(mystery.weapon_locations_outro[language]) | ||
|
|
||
| sub_bullets = [] | ||
| for clue in mystery.final_locations_clues: | ||
| clue = create_template(clue[language]).substitute(names_html) | ||
| sub_bullets.append(clue) | ||
|
|
||
| final_locations_bullets = mystery.final_locations_intro[language] | ||
| final_locations_bullets += get_bullet_list(sub_bullets) | ||
| bullets.append(final_locations_bullets) | ||
| bullets.append(NoOneElseStatement().string()[language]) | ||
|
|
||
| initial_clues = get_bullet_list(bullets) | ||
|
|
||
| additional_clues = [] | ||
|
|
||
| for i, clue in enumerate(mystery.additional_clues): | ||
| clue = replace_emojis(create_template(clue[language]).substitute(names_html)) | ||
| additional_clues.append(clue) | ||
|
|
||
| additional_clues_enumeration = get_enumeration_list(additional_clues) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in clue processing.
The clue processing logic is repeated for different types of clues. Consider extracting this into a reusable function.
Apply this diff:
+def process_clues(clues, language, names_html):
+ """Process a list of clues with template substitution.
+
+ Args:
+ clues: List of clue objects
+ language: Current language
+ names_html: Dictionary of HTML-formatted names
+
+ Returns:
+ List of processed clue strings
+ """
+ return [
+ create_template(clue[language]).substitute(names_html)
+ for clue in clues
+ ]
+
-bullets = []
-for i, clue in enumerate(mystery.initial_clues):
- clue = create_template(clue[language]).substitute(names_html)
- bullets.append(clue)
+bullets = process_clues(mystery.initial_clues, language, names_html)
-sub_bullets = []
-for clue in mystery.weapon_locations_clues:
- clue = create_template(clue[language]).substitute(names_html)
- sub_bullets.append(clue)
+sub_bullets = process_clues(
+ mystery.weapon_locations_clues,
+ language,
+ names_html
+)
weapon_locations_bullets = mystery.weapon_locations_intro[language]
weapon_locations_bullets += get_bullet_list(sub_bullets, language)
bullets.append(weapon_locations_bullets)
bullets.append(mystery.weapon_locations_outro[language])
-sub_bullets = []
-for clue in mystery.final_locations_clues:
- clue = create_template(clue[language]).substitute(names_html)
- sub_bullets.append(clue)
+sub_bullets = process_clues(
+ mystery.final_locations_clues,
+ language,
+ names_html
+)
final_locations_bullets = mystery.final_locations_intro[language]
final_locations_bullets += get_bullet_list(sub_bullets)
bullets.append(final_locations_bullets)
bullets.append(NoOneElseStatement().string()[language])
initial_clues = get_bullet_list(bullets)
-additional_clues = []
-for i, clue in enumerate(mystery.additional_clues):
- clue = replace_emojis(create_template(clue[language]).substitute(names_html))
- additional_clues.append(clue)
+additional_clues = [
+ replace_emojis(create_template(clue[language]).substitute(names_html))
+ for clue in mystery.additional_clues
+]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bullets = [] | |
| for i, clue in enumerate(mystery.initial_clues): | |
| clue = create_template(clue[language]).substitute(names_html) | |
| bullets.append(clue) | |
| sub_bullets = [] | |
| for clue in mystery.weapon_locations_clues: | |
| clue = create_template(clue[language]).substitute(names_html) | |
| sub_bullets.append(clue) | |
| weapon_locations_bullets = mystery.weapon_locations_intro[language] | |
| weapon_locations_bullets += get_bullet_list(sub_bullets, language) | |
| bullets.append(weapon_locations_bullets) | |
| bullets.append(mystery.weapon_locations_outro[language]) | |
| sub_bullets = [] | |
| for clue in mystery.final_locations_clues: | |
| clue = create_template(clue[language]).substitute(names_html) | |
| sub_bullets.append(clue) | |
| final_locations_bullets = mystery.final_locations_intro[language] | |
| final_locations_bullets += get_bullet_list(sub_bullets) | |
| bullets.append(final_locations_bullets) | |
| bullets.append(NoOneElseStatement().string()[language]) | |
| initial_clues = get_bullet_list(bullets) | |
| additional_clues = [] | |
| for i, clue in enumerate(mystery.additional_clues): | |
| clue = replace_emojis(create_template(clue[language]).substitute(names_html)) | |
| additional_clues.append(clue) | |
| additional_clues_enumeration = get_enumeration_list(additional_clues) | |
| def process_clues(clues, language, names_html): | |
| """Process a list of clues with template substitution. | |
| Args: | |
| clues: List of clue objects | |
| language: Current language | |
| names_html: Dictionary of HTML-formatted names | |
| Returns: | |
| List of processed clue strings | |
| """ | |
| return [ | |
| create_template(clue[language]).substitute(names_html) | |
| for clue in clues | |
| ] | |
| bullets = process_clues(mystery.initial_clues, language, names_html) | |
| sub_bullets = process_clues( | |
| mystery.weapon_locations_clues, | |
| language, | |
| names_html | |
| ) | |
| weapon_locations_bullets = mystery.weapon_locations_intro[language] | |
| weapon_locations_bullets += get_bullet_list(sub_bullets, language) | |
| bullets.append(weapon_locations_bullets) | |
| bullets.append(mystery.weapon_locations_outro[language]) | |
| sub_bullets = process_clues( | |
| mystery.final_locations_clues, | |
| language, | |
| names_html | |
| ) | |
| final_locations_bullets = mystery.final_locations_intro[language] | |
| final_locations_bullets += get_bullet_list(sub_bullets) | |
| bullets.append(final_locations_bullets) | |
| bullets.append(NoOneElseStatement().string()[language]) | |
| initial_clues = get_bullet_list(bullets) | |
| additional_clues = [ | |
| replace_emojis(create_template(clue[language]).substitute(names_html)) | |
| for clue in mystery.additional_clues | |
| ] | |
| additional_clues_enumeration = get_enumeration_list(additional_clues) |
🧰 Tools
🪛 Ruff
69-69: Loop control variable i not used within loop body
(B007)
97-97: Loop control variable i not used within loop body
(B007)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
mystery_o_matic/output/latex/__init__.py (3)
9-21: Consider extracting name processing logic into helper functions.The character and name processing logic could be more maintainable if extracted into dedicated helper functions.
+def process_character_names(characters, format_func): + """Process character names with the given format function.""" + return { + f"CHAR{i + 1}": format_func(char) + for i, char in enumerate(characters) + } + names_html = {} -for i, char in enumerate(mystery.get_characters()): - names_html["CHAR" + str(i + 1)] = get_char_name(char) +names_html.update(process_character_names(mystery.get_characters(), get_char_name)) names_html["SUS0"] = suspects[0].capitalize() names_html["SUS1"] = suspects[1].capitalize() names_txt = {} -for i, char in enumerate(mystery.get_characters()): - names_txt["CHAR" + str(i + 1)] = char.lower() +names_txt.update(process_character_names(mystery.get_characters(), str.lower))
23-39: Simplify location and weapon mapping logic.The mapping logic could be more concise and maintainable using dictionary comprehensions.
-final_locations_map = {} -for c, p in mystery.final_locations.items(): - c = create_template(c).substitute(names_txt) - p = create_template(p).substitute(names_txt) - final_locations_map[c] = p +final_locations_map = { + create_template(c).substitute(names_txt): create_template(p).substitute(names_txt) + for c, p in mystery.final_locations.items() +} -representations_map = {} -for l, r in locations.representations.items(): - l = create_template("$" + l).substitute(names_txt) - representations_map[l] = r +representations_map = { + create_template("$" + loc).substitute(names_txt): rep + for loc, rep in locations.representations.items() +} -weapons_map = {} -for l, w in locations.weapon_locations.items(): - l = create_template("$" + l).substitute(names_txt) - weapons_map[w] = l +weapons_map = { + w: create_template("$" + loc).substitute(names_txt) + for loc, w in locations.weapon_locations.items() +}🧰 Tools
🪛 Ruff
30-30: Ambiguous variable name:
l(E741)
31-31: Ambiguous variable name:
l(E741)
36-36: Ambiguous variable name:
l(E741)
37-37: Ambiguous variable name:
l(E741)
118-144: Organize template arguments preparation.The argument preparation logic could be more organized and efficient by grouping related arguments together.
+def prepare_character_args(characters, names_html): + """Prepare character-related template arguments.""" + return { + f"CHAR{i+1}": names_html[f"CHAR{i+1}"] + for i in range(len(characters)) + } +def prepare_room_args(locations, language): + """Prepare room-related template arguments.""" + args = {} + for room in locations.names[language]: + if room in locations.rindices: + index = locations.rindices[room] + args[index + "REP"] = get_emoji_name(locations.representations[index]) + return args args = {} args["introLocation"] = introLocation args["initialClues"] = initial_clues args["additionalClues"] = additional_clues_enumeration - -for i in range(len(mystery.get_characters())): - args[f"CHAR{i+1}"] = names_html[f"CHAR{i+1}"] - -for room in locations.names[language]: - if room not in locations.rindices: - continue - index = locations.rindices[room] - args[index + "REP"] = get_emoji_name(locations.representations[index]) +args.update(prepare_character_args(mystery.get_characters(), names_html)) +args.update(prepare_room_args(locations, language))🧰 Tools
🪛 Ruff
139-139: Ambiguous variable name:
l(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
mystery_o_matic/output/latex/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff
mystery_o_matic/output/latex/__init__.py
30-30: Ambiguous variable name: l
(E741)
31-31: Ambiguous variable name: l
(E741)
36-36: Ambiguous variable name: l
(E741)
37-37: Ambiguous variable name: l
(E741)
69-69: Loop control variable i not used within loop body
(B007)
97-97: Loop control variable i not used within loop body
(B007)
139-139: Ambiguous variable name: l
(E741)
| for language in languages: | ||
|
|
||
| if language == "en": | ||
| names_html["NOBODY"] = "nobody" | ||
| names_txt["NOBODY"] = "nobody" | ||
| elif language == "es": | ||
| names_html["NOBODY"] = "nadie" | ||
| names_txt["NOBODY"] = "nadie" | ||
| else: | ||
| raise ValueError("Unknown language: " + language) | ||
|
|
||
| characters = list(map(lambda char: char.capitalize(), mystery.get_characters())) | ||
| introLocation = ", ".join(characters[:-1]) + " and " + characters[-1] | ||
| introLocation += locations.intro[language] | ||
|
|
||
| for room, name in locations.names[language].items(): | ||
| if room not in locations.rindices: | ||
| continue # skip any missing place | ||
| index = locations.rindices[room] | ||
| names_html[index] = name + " (" + get_emoji_name(locations.representations[index]) + ")" | ||
| for weapon, label in weapon_labels[language].items(): | ||
| if weapon not in weapons: | ||
| continue | ||
| if language == "es": | ||
| label = label.capitalize() | ||
| names_html[weapon.replace("$", "")] = label + " (" + get_emoji_name(weapons[weapon]) + ")" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve language handling extensibility.
The current language handling is hardcoded and not easily extensible. Consider using a configuration-based approach.
+LANGUAGE_CONFIGS = {
+ "en": {"nobody": "nobody"},
+ "es": {"nobody": "nadie"}
+}
for language in languages:
- if language == "en":
- names_html["NOBODY"] = "nobody"
- names_txt["NOBODY"] = "nobody"
- elif language == "es":
- names_html["NOBODY"] = "nadie"
- names_txt["NOBODY"] = "nadie"
- else:
+ if language not in LANGUAGE_CONFIGS:
raise ValueError("Unknown language: " + language)
+
+ config = LANGUAGE_CONFIGS[language]
+ names_html["NOBODY"] = config["nobody"]
+ names_txt["NOBODY"] = config["nobody"]Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependencies
emojipackage as a requirement for improved emoji handling in LaTeX output.