Extract memo information from PNG file Comment tag and display in Knitting Progress window #779
Conversation
|
""" WalkthroughThis change implements the propagation and display of "memo" data throughout the AYAB application. Memos are now extracted from PNG metadata in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GUI
participant AyabImage
participant Engine
participant Pattern
participant Control
participant KnitProgress
User->>GUI: Load PNG with memo metadata
GUI->>AyabImage: Open image
AyabImage->>AyabImage: Extract memos from PNG metadata
GUI->>Engine: knit_config(AyabImage)
Engine->>AyabImage: Access image and memos
Engine->>Pattern: Create Pattern(image, memos, config)
Engine->>Control: start(Pattern, memos, options, operation)
Control->>Control: Integrate memos into logging and status
Control->>KnitProgress: Update progress with memo info
KnitProgress->>GUI: Display memo column in progress table
Possibly related issues
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 2
🧹 Nitpick comments (5)
src/main/python/main/ayab/gui_fsm.py (1)
148-148: Formatting: Added whitespace line.This is a minor formatting change adding a blank line for better readability.
src/main/python/main/ayab/image.py (1)
116-129: Implementation for extracting memo information from PNG metadata.The implementation looks good overall, but there are some improvements that could be made:
- Consider adding validation for integer parsing to handle non-integer characters in the comment
- There's no error handling if int() conversion fails
- The memo list size is dependent on the comment length, which might not be ideal
Here's an improved implementation with better error handling:
if suffix == ".png": # check metadata for memo information self.image.load() if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information self.memo = [] - for i in range(len(comment) - 5): - self.memo.append(int(comment[i + 5])) + memo_data = comment[5:] # Extract everything after "AYAB:" + try: + # Parse each character as an integer + for char in memo_data: + if char.isdigit(): + self.memo.append(int(char)) + else: + logging.warning(f"Skipping non-digit character '{char}' in memo data") + except Exception as e: + logging.error(f"Error parsing memo data: {e}") # report metadata logging.info("File metadata Comment tag: " + comment) logging.info("File memo information: " + str(self.memo))src/main/python/main/ayab/engine/control.py (2)
237-240: Improve exception handling pattern.The current implementation uses a bare
exceptblock which silently swallows all exceptions. This can hide potential issues and make debugging difficult.Consider using
contextlib.suppressfor clearer intention or specifying the exact exception type you expect:-try: - msg = msg + " memo: " + str(self.memo[self.pat_row]) -except: - pass +import contextlib + +with contextlib.suppress(IndexError): + msg = msg + " memo: " + str(self.memo[self.pat_row])Or with explicit exception type:
-try: - msg = msg + " memo: " + str(self.memo[self.pat_row]) -except: - pass +try: + msg = msg + " memo: " + str(self.memo[self.pat_row]) +except IndexError: + pass # Skip memo logging if index out of range🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
274-277: Improve exception handling for status memo update.Similar to the previous issue, this code uses a bare
exceptblock.Apply the same improvement pattern here:
-try: - self.status.memo = self.memo[self.pat_row] -except: - self.status.memo = 0 +try: + self.status.memo = self.memo[self.pat_row] +except IndexError: + self.status.memo = 0 # Default value when memo index is out of range🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
src/main/python/main/ayab/knitprogress.py (1)
39-39: Remove redundant TODO comment.This TODO comment about displaying memo information is no longer needed since the feature is being implemented in this PR.
-# TODO (TP): display memo information
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/python/main/ayab/engine/control.py(4 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/pattern.py(2 hunks)src/main/python/main/ayab/engine/status.py(3 hunks)src/main/python/main/ayab/gui_fsm.py(2 hunks)src/main/python/main/ayab/image.py(4 hunks)src/main/python/main/ayab/knitprogress.py(7 hunks)src/main/python/main/ayab/pattern_import.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
src/main/python/main/ayab/engine/status.py (1)
src/main/python/main/ayab/engine/pattern.py (1)
memo(204-205)
src/main/python/main/ayab/gui_fsm.py (1)
src/main/python/main/ayab/engine/engine.py (1)
knit_config(122-160)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/engine/pattern.py (1)
memo(204-205)src/main/python/main/ayab/utils.py (1)
display_blocking_popup(44-65)
src/main/python/main/ayab/engine/control.py (3)
src/main/python/main/ayab/engine/pattern.py (2)
memo(204-205)Pattern(33-208)src/main/python/main/ayab/engine/options.py (1)
OptionsTab(59-252)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
src/main/python/main/ayab/engine/pattern.py (2)
src/main/python/main/ayab/image.py (1)
AyabImage(46-205)src/main/python/main/ayab/engine/options.py (2)
OptionsTab(59-252)Alignment(255-266)
src/main/python/main/ayab/knitprogress.py (1)
src/main/python/main/ayab/engine/pattern.py (1)
memo(204-205)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
🔇 Additional comments (26)
src/main/python/main/ayab/pattern_import.py (1)
8-8: Appropriate TODO comment for future implementation.This comment aligns with the PR's objective to extract memo information from PNG files and display it in the Knitting Progress window.
src/main/python/main/ayab/engine/status.py (1)
164-164: Appropriate copying of memo value in Status.copy method.The copy method correctly assigns the memo from the source status object.
src/main/python/main/ayab/gui_fsm.py (1)
110-111: Updated parameter to match new knit_config method signature.This change correctly passes the entire
ayabimageobject instead of just the image attribute, allowingknit_configto access the newly added memo information.src/main/python/main/ayab/image.py (3)
59-59: Added memo attribute to store metadata information.The addition of the
memolist allows storing integers extracted from PNG metadata.
90-93: Removed FIXME comments from error handling.The removal of FIXME comments simplifies the code while maintaining the same functionality.
96-98: Removed FIXME comments from error handling.Similar to the previous change, this simplifies the error handling code.
src/main/python/main/ayab/engine/control.py (3)
64-64: Added new member variable for memo data.The addition of this attribute supports the main PR objective of extracting and displaying memo information from PNG files.
89-90: Updated method signature to include memo parameter.This change appropriately propagates the memo information from the engine to the control layer.
97-97: Initialized memo member variable.Correctly initializes the class's memo attribute from the parameter.
src/main/python/main/ayab/engine/pattern.py (5)
29-29: Added import for AyabImage class.This import supports the updated constructor parameter type.
34-37: Updated constructor to use AyabImage instead of Image.Image.This change correctly modifies the parameter type and accesses the image through the AyabImage.image property.
38-38: Added memo attribute to store memo data.This attribute properly stores the memo information from the AyabImage object.
39-40: Added explicit type annotations.These annotations improve code clarity and provide better type checking.
203-205: Added memo property to access the memo data.This property correctly exposes the private memo attribute, following Python's property access pattern.
src/main/python/main/ayab/engine/engine.py (7)
38-38: Added import for AyabImage class.This import supports the updated knit_config parameter type.
54-54: Added memo class attribute.This attribute declaration improves code readability by explicitly stating the class attribute's type.
71-71: Initialized memo instance attribute.Properly initializes the memo as an empty list.
122-122: Updated knit_config method signature.Changed parameter from Image.Image to AyabImage to support the memo feature.
131-131: Updated image transformation code.Now correctly accesses the image through the AyabImage.image property.
135-136: Updated pattern initialization and added memo extraction.These lines properly initialize the pattern with the AyabImage object and extract the memo information.
173-173: Updated control.start call to include memo parameter.This change correctly passes the memo information to the control layer.
src/main/python/main/ayab/knitprogress.py (5)
49-52: Updated color constants.The changes to existing color constants and addition of new ones provide better visual representation for the memo information.
94-94: Updated UI state change detection.This change correctly detects when the memo information has changed and triggers a UI update.
142-142: Added memo column to the progress table.This line appends a memo item to the columns list, allowing the memo information to be displayed in the UI.
175-178: Added header for memo column.These lines correctly set up the memo column header with appropriate styling.
275-284: Added memo cell renderer.This method creates a table cell for displaying the memo value, with appropriate styling and handling of zero values.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/python/main/ayab/image.py (2)
59-59: Add type annotations for consistencyThe
memosattribute is defined with justlisttype, but based on the usage in the file it's meant to store integers. Consider using a more specific type annotation likelist[int]for better type checking and code clarity.- self.memos: list = [] + self.memos: list[int] = []
124-136: Consider using a more flexible memo formatThe current implementation only allows for single-digit integers (0-9) in memos. Consider using a delimiter like commas or spaces to support multi-digit values, which would be more versatile for storing memo information.
if comment.startswith("AYAB:"): # update memo information self.memos = [] - for i in range(len(comment) - 5): - self.memos.append(int(comment[i + 5])) + # Parse comma-separated values after "AYAB:" + memo_str = comment[5:] + try: + for val in memo_str.split(','): + if val.strip(): + self.memos.append(int(val.strip())) + except ValueError as e: + logging.error(f"Error parsing memo value: {str(e)}")src/main/python/main/ayab/engine/control.py (3)
237-240: Improve exception handlingThe current code uses a bare
exceptclause which catches all exceptions and silently ignores them. This can hide bugs and make debugging difficult.try: msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + except IndexError: + self.logger.debug(f"No memo available for pattern row {self.pat_row}") + except Exception as e: + self.logger.error(f"Error accessing memo data: {str(e)}")Alternatively, use
contextlib.suppressas suggested by the static analysis:- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + from contextlib import suppress + with suppress(IndexError): + msg = msg + " memo: " + str(self.memos[self.pat_row])🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
274-277: Improve exception handling for status updateSimilar to the previous comment, this code uses a bare
exceptclause. Specify the expected exception type and provide a log message to help with debugging.try: self.status.memo = self.memos[self.pat_row] - except: + except IndexError: + self.logger.debug(f"No memo available for pattern row {self.pat_row}, defaulting to 0") self.status.memo = 0 + except Exception as e: + self.logger.error(f"Error setting memo status: {str(e)}") + self.status.memo = 0🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
294-294: Consider more selective loggingLogging the entire memos list on every status update could be excessive and make logs difficult to read, especially for large lists.
- self.logger.debug(str(self.memos)) + # Only log if needed for debugging specific issues + if self.logger.isEnabledFor(logging.DEBUG): + self.logger.debug(f"Memos (count: {len(self.memos)}): {str(self.memos)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/python/main/ayab/engine/control.py(5 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/pattern.py(2 hunks)src/main/python/main/ayab/image.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/python/main/ayab/engine/engine.py
- src/main/python/main/ayab/engine/pattern.py
🧰 Additional context used
🧬 Code Definitions (2)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/engine/pattern.py (1)
memos(204-205)src/main/python/main/ayab/utils.py (1)
display_blocking_popup(44-65)
src/main/python/main/ayab/engine/control.py (3)
src/main/python/main/ayab/engine/pattern.py (2)
memos(204-205)Pattern(33-208)src/main/python/main/ayab/engine/options.py (1)
OptionsTab(59-252)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
🔇 Additional comments (4)
src/main/python/main/ayab/image.py (1)
63-69: The clone method looks goodNice implementation of the clone method to properly copy all AyabImage attributes including the new memos list.
src/main/python/main/ayab/engine/control.py (3)
64-64: Good use of type annotationThe type annotation
list[int]for thememosattribute clearly indicates the expected type of elements in the list, which is better than just usinglist.
89-90: Function signature update looks goodCorrectly updated the function signature to include the new
memosparameter, maintaining consistency with other parameters.
97-97: Properly storing the memos parameterThe implementation correctly saves the passed memos parameter to the instance variable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/python/main/ayab/image.py (1)
116-132:⚠️ Potential issueImprove error handling for memo parsing
The current implementation attempts to parse digits from the Comment tag but uses a bare
exceptclause which is flagged by static analysis. Additionally, this approach silently converts parsing errors to zeros, which could lead to unexpected behavior.if suffix == ".png": # check metadata for memo information self.image.load() if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information self.memos = [] - for i in range(len(comment) - 5): - try: - self.memos.append(int(comment[i + 5])) - except: - self.memos.append(0) + try: + for i in range(len(comment) - 5): + char = comment[i + 5] + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}") # report metadata logging.info("File metadata Comment tag: " + comment) logging.info("File memo information: " + str(self.memos))🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
🧹 Nitpick comments (2)
src/main/python/main/ayab/image.py (2)
116-132: Consider supporting multi-digit numbers in memo formatThe current implementation assumes each character after "AYAB:" is a single-digit integer. If you need to support multi-digit numbers or other structured data formats, you may want to use a different parsing approach.
For example, you could use a more structured format like "AYAB:1,2,3,10,20" and parse it with:
if suffix == ".png": # check metadata for memo information self.image.load() if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information self.memos = [] - for i in range(len(comment) - 5): - try: - self.memos.append(int(comment[i + 5])) - except: - self.memos.append(0) + try: + # Parse comma-separated values after "AYAB:" + memo_str = comment[5:].strip() + if memo_str: + for num_str in memo_str.split(','): + if num_str.strip().isdigit(): + self.memos.append(int(num_str.strip())) + else: + logging.warning(f"Non-digit value '{num_str}' found in memo data") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}") # report metadata logging.info("File metadata Comment tag: " + comment) logging.info("File memo information: " + str(self.memos))🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
119-131: Consider documenting the AYAB memo formatThe code adds parsing for a custom "AYAB:" memo format in PNG comments, but this format isn't documented in the code. Consider adding a comment explaining the expected format and meaning of these memo values.
if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) + # The AYAB memo format uses "AYAB:" prefix followed by single-digit integers + # representing specific knitting instructions or metadata if comment.startswith("AYAB:"): # update memo information🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/image.py(4 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/engine/pattern.py (1)
memos(203-204)src/main/python/main/ayab/utils.py (1)
display_blocking_popup(44-65)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
🔇 Additional comments (2)
src/main/python/main/ayab/image.py (2)
59-59: New attribute added for memo informationAdded
memosattribute to store the integers extracted from PNG file Comment metadata. This is well-typed aslist[int]and properly initialized as an empty list.
92-92: Code cleanup: FIXME comments removedRemoved FIXME comments from display_blocking_popup calls, making the code cleaner.
Also applies to: 97-97
|
Why not using a JSON string to store memo ?
Why not use an new exif or native iTXt "ayab" key rather than use a "Comment" key with value starting wth "AYAB:" ? See https://discord.com/channels/1079442610332110849/1094306201451384944/1355294497696911410 |
There is no need from the point of view of feature parity, it is harder to implement, it makes manual editing more difficult. In any case, this is still a draft. It might turn out that the way that DAK implements memo information requires a more flexible format and if so we can change it or add a new one. |
|
Not sure I understand why you said it is harder to implement (on the contrary) and that it makes manual editing more difficult (same to me), see below Current code if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0:
comment = str(self.image.info["Comment"])
if comment.startswith("AYAB:"):
# update memo information
self.memos = []
for i in range(len(comment) - 5):
self.memos.append(int(comment[i + 5]))Use JSON instead of raw/single digit data info["Comment"] = 'AYAB:{"memo":[1,12,23,34,45], "color":[1,2,3,4,5], "carriage":["K", "K", "L", "L", "G"], "version":1}' if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0:
comment = str(self.image.info["Comment"])
if comment.startswith("AYAB:"):
metadata = json.loads(comment[5:])
# update memo information
self.memos = metadata["memo"]=> no parsing in the code anymore, extend beyond "memo" Use JSON with a dedicated metadata key info["AYAB"] = '{"memo":[1,12,23,34,45], "color":[1,2,3,4,5], "carriage":["K", "K", "L", "L", "G"], "version":1}' if "AYAB" in self.image.info:
metadata = json.loads(self.image.info["AYAB"])
# update memo information
self.memos = metadata["memo"]=> Same as above + no need to check for a string prefix Code is shorter/generic and give you much more flexibility for free ? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/main/python/main/ayab/image.py (1)
117-132: Improve error handling in memo parsingWhile the implementation correctly extracts memo information from PNG metadata, it uses a bare
exceptclause which can mask unexpected errors.if comment.startswith("AYAB:"): # update memo information for i in range(len(comment) - 5): try: self.memos.append(int(comment[i + 5])) - except: + except ValueError: self.memos.append(0)Additionally, consider adding a comment explaining what these memo values represent or are used for, to help future developers understand the feature's purpose.
🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
src/main/python/main/ayab/engine/control.py (2)
237-240: Use specific exception type for robust error handlingThe current implementation uses a bare
exceptclause which can catch and silence unexpected exceptions.Consider using a specific exception type and Python's
contextlib.suppressfor cleaner code:- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + from contextlib import suppress + with suppress(IndexError): + msg = msg + " memo: " + str(self.memos[self.pat_row])This approach makes it clear that we're specifically handling index out-of-bounds errors and provides a more pythonic solution.
🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
274-277: Improve exception handling for setting status memoSimilar to the previous comment, this code uses a bare
exceptclause.- try: - self.status.memo = self.memos[self.pat_row] - except: - self.status.memo = 0 + try: + self.status.memo = self.memos[self.pat_row] + except IndexError: + self.status.memo = 0This makes it explicit that we're handling out-of-bounds index errors specifically, improving code clarity and safety.
🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
src/main/python/main/ayab/knitprogress.py (2)
39-39: Remove completed TODO commentThe TODO comment about displaying memo information can be removed since the implementation is now complete.
-# TODO (TP): display memo information
277-286: Clean implementation of memo cell renderingThe
__memomethod is well-implemented, displaying memo values greater than 0 and using appropriate styling for visual clarity.Consider adding a tooltip to explain what the memo values represent to users, especially as this appears to be a new feature:
def __memo( self, memo: int, ) -> QTableWidgetItem: cell = QTableWidgetItem() cell.setBackground(QBrush(QColor(f"#{self.smoke:06x}"))) if memo > 0: cell.setText(str(memo)) + cell.setToolTip("Memo information from image metadata") return cell
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/python/main/ayab/engine/control.py(4 hunks)src/main/python/main/ayab/image.py(5 hunks)src/main/python/main/ayab/knitprogress.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/main/python/main/ayab/knitprogress.py (1)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)knit_start_needle(183-184)
src/main/python/main/ayab/engine/control.py (2)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)Pattern(32-207)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/engine/pattern.py (1)
memos(203-204)src/main/python/main/ayab/utils.py (1)
display_blocking_popup(44-65)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
🔇 Additional comments (10)
src/main/python/main/ayab/image.py (2)
59-59: Addition of a new memo featureThe introduction of the
memoslist as a class attribute provides a clean way to store integer metadata associated with the image. This attribute will be propagated through the system to display in the UI.
106-106: Good initialization practiceProperly clearing the memos list at the beginning of the
__openmethod ensures a clean state before processing a new image.src/main/python/main/ayab/engine/control.py (3)
64-64: Addition of memos attributeThe new
memosclass attribute extends theControlclass to support the memo feature. This is a clean way to propagate memo information through the control system.
89-89: Updated method signature to include memosThe
startmethod signature now includes thememosparameter, allowing the memo information to be passed from the engine to the control component.
97-97: Setting the memos attributeProperly initializing the
memosattribute in thestartmethod ensures that memo information is available throughout the control component's lifecycle.src/main/python/main/ayab/knitprogress.py (5)
49-52: Updated color constantsThe modifications to existing color constants and addition of new ones provide a consistent color scheme for the UI components, including the new memo column.
84-84: Dynamic memo column visibilityGood approach to conditionally show the memo column based on the presence of memo data. This ensures the UI remains clean when no memo information is available.
95-95: UI state tracking for memo changesAdding memo status to the UI state change tracking ensures that the progress display updates correctly when memo information changes.
143-144: Conditional memo column additionThe implementation correctly adds the memo column only when memo information is available, keeping the UI clean and focused.
175-189: Good attention to UI layout adjustmentThe code properly adjusts the needle calculation and column headers to account for the presence of the memo column. The use of color-coded headers enhances visual distinction.
|
The format implemented in the code is consistent with AllYarnsAreBeautiful/ayab-patterns#7 If you want to change one, you'll want to change the other. |
|
I've added a commit that points the patterns submodule to include the already-enhanced patterns (up to number 124 in I have a couple of nits to pick with the code as it is (mainly the issues reported by SonarQube above) but the UI part feels ready for initial testing. So I've triggered a build from this PR so that we can collect tester feedback: https://github.com/jonathanperret/ayab-desktop/releases/tag/1.0.0-memo1 |
|
Writing some Discord feedback here so we can have it in one place:
|
|
A couple more issues that I found/remembered:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/python/main/ayab/image.py (1)
117-132:⚠️ Potential issueFix type mismatch and bare except in memo extraction.
Several issues in the memo extraction implementation need to be fixed:
- The
memoslist is defined to store integers (list[int]), but you're appending string characters- Using a bare
exceptclause is discouraged- The error handling logic doesn't maintain type consistency
if suffix == ".png": # check metadata for memo information self.image.load() if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information - for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + for i in range(len(comment) - 5): + try: + char = comment[i + 5] + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo information, ignoring") + except Exception as e: + logging.warning(f"Error processing memo character: {e}") # report metadata logging.info("File metadata Comment tag: " + comment) logging.info("File memo information: " + str(self.memos))🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
🪛 GitHub Actions: Python Tests
[error] 126-126: Argument 1 to 'append' of 'list' has incompatible type 'str'; expected 'int' [arg-type]
[error] 128-128: Argument 1 to 'append' of 'list' has incompatible type 'str'; expected 'int' [arg-type]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/python/main/ayab/image.py(5 hunks)src/main/python/main/ayab/knitprogress.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/main/python/main/ayab/knitprogress.py (1)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)knit_start_needle(183-184)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/engine/pattern.py (1)
memos(203-204)src/main/python/main/ayab/utils.py (1)
display_blocking_popup(44-65)
🪛 GitHub Actions: Python Tests
src/main/python/main/ayab/knitprogress.py
[error] 283-283: Argument 1 to 'len' has incompatible type 'int'; expected 'Sized' [arg-type]
[error] 283-283: Non-overlapping equality check (left operand type: 'int', right operand type: 'Literal['0']') [comparison-overlap]
[error] 284-284: Argument 1 to 'setText' of 'QTableWidgetItem' has incompatible type 'int'; expected 'str' [arg-type]
src/main/python/main/ayab/image.py
[error] 126-126: Argument 1 to 'append' of 'list' has incompatible type 'str'; expected 'int' [arg-type]
[error] 128-128: Argument 1 to 'append' of 'list' has incompatible type 'str'; expected 'int' [arg-type]
🪛 Ruff (0.8.2)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
🔇 Additional comments (7)
src/main/python/main/ayab/image.py (2)
59-59: Newmemoslist attribute properly defined.The addition of the
memoslist attribute to store integers is a good implementation choice for capturing the memo information from image metadata.
106-106: LGTM!Properly initializing the memos list at the beginning of the
__openmethod ensures a clean state before processing any new image.src/main/python/main/ayab/knitprogress.py (5)
51-52: LGTM!Defining color constants for grey and smoke provides good consistency for UI styling.
84-84: LGTM!The flag
show_memo_columnis a good approach to conditionally display the memo column only when memo information is available.
95-95: LGTM!Adding
status.memoto the UI state change check ensures the progress display updates when memo information changes.
143-144: LGTM!Conditionally adding the memo column to the display based on the
show_memo_columnflag is a good implementation.
175-189: LGTM!The adjustments to needle calculation and column headers are correctly implemented to accommodate the memo column when needed.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/python/main/ayab/image.py (1)
124-129:⚠️ Potential issueImprove memo data extraction and error handling
The current implementation has several issues:
- The bare
exceptclause is too broad- Converting string characters to integers can throw exceptions
- The default value of '0' is a string, but should be an integer based on type expectations
- for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + for i in range(len(comment) - 5): + try: + # Convert character to integer + char = comment[i + 5] + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo, defaulting to 0") + self.memos.append(0) + except Exception as e: + logging.error(f"Error processing memo at position {i}: {str(e)}") + self.memos.append(0)🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
🧹 Nitpick comments (3)
src/main/python/main/ayab/image.py (1)
130-131: Add comment format verification loggingIt would be helpful to log whether the extracted memo information matches expectations, especially given the PR discussion about JSON vs. simple format.
# report metadata logging.info("File metadata Comment tag: " + comment) - logging.info("File memo information: " + str(self.memos)) + logging.info(f"File memo information: {self.memos} (extracted {len(self.memos)} memo values)") + if not all(isinstance(m, int) for m in self.memos): + logging.warning("Memo data contains non-integer values, which may cause issues")src/main/python/main/ayab/engine/control.py (1)
237-240: Improve error handling for memo loggingInstead of using a bare
exceptclause with a silent pass, usecontextlib.suppressfor better readability or provide proper error handling.+ from contextlib import suppress - try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + with suppress(IndexError): + msg = msg + " memo: " + str(self.memos[self.pat_row])Alternatively, with explicit error handling:
- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + try: + msg = msg + " memo: " + str(self.memos[self.pat_row]) + except IndexError: + self.logger.debug(f"No memo available for row {self.pat_row}") + except Exception as e: + self.logger.warning(f"Error accessing memo for row {self.pat_row}: {str(e)}")🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
src/main/python/main/ayab/engine/pattern.py (1)
32-33: Consider JSON format for memo dataBased on the PR discussion, using JSON for storing memo data would allow for more flexibility and easier extension. While the current implementation works for single-digit values, JSON would support more complex data.
As suggested in the PR comments, JSON would allow you to:
- Store more complex data types (not just single digits)
- Add versioning, carriage information, colors, etc.
- Simplify parsing with
json.loads- Remove the limitation of single-digit numbers
This could be implemented while maintaining backward compatibility by detecting the format (JSON vs. simple string) during parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/python/main/ayab/engine/control.py(4 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/pattern.py(2 hunks)src/main/python/main/ayab/engine/status.py(3 hunks)src/main/python/main/ayab/image.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/python/main/ayab/engine/status.py
- src/main/python/main/ayab/engine/engine.py
🧰 Additional context used
🧬 Code Definitions (3)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/engine/pattern.py (1)
memos(203-204)src/main/python/main/ayab/utils.py (1)
display_blocking_popup(44-65)
src/main/python/main/ayab/engine/pattern.py (1)
src/main/python/main/ayab/engine/options.py (2)
OptionsTab(59-252)Alignment(255-266)
src/main/python/main/ayab/engine/control.py (3)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)Pattern(32-207)src/main/python/main/ayab/engine/options.py (1)
OptionsTab(59-252)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
🪛 GitHub Actions: Python Tests
src/main/python/main/ayab/engine/pattern.py
[error] 37-37: Incompatible types in assignment (expression has type 'list[int]', variable has type 'list[str]')
[error] 204-204: Incompatible return value type (got 'list[str]', expected 'list[int]')
src/main/python/main/ayab/engine/control.py
[error] 97-97: Incompatible types in assignment (expression has type 'list[int]', variable has type 'list[str]')
[error] 277-277: Incompatible types in assignment (expression has type 'int', variable has type 'str')
🔇 Additional comments (1)
src/main/python/main/ayab/engine/pattern.py (1)
202-204: Fix return type annotation for memos propertyThe memos property is declared to return
list[int]but actually returns__memoswhich is inconsistently typed aslist[str].This property looks good once the type annotation for
__memosis fixed to match the return type annotation.🧰 Tools
🪛 GitHub Actions: Python Tests
[error] 204-204: Incompatible return value type (got 'list[str]', expected 'list[int]')
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/main/python/main/ayab/engine/control.py (3)
64-64:⚠️ Potential issueFix type annotation for memos attribute
The memos attribute is defined with the wrong type, causing pipeline failures. It should be
list[int]to match the parameter type in thestartmethod.- memos: list[str] + memos: list[int]
274-277:⚠️ Potential issueFix status memo type mismatch and improve error handling
There's a type mismatch here:
self.status.memois expected to be a string but is being assigned an integer value of 0. Additionally, the bareexceptshould be avoided.- try: - self.status.memo = self.memos[self.pat_row] - except: - self.status.memo = 0 + try: + self.status.memo = str(self.memos[self.pat_row]) + except IndexError: + self.status.memo = "0" + except Exception as e: + self.logger.warning(f"Error setting memo status: {str(e)}") + self.status.memo = "0"🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
🪛 GitHub Actions: Python Tests
[error] 277-277: Incompatible types in assignment (expression has type 'int', variable has type 'str') [assignment]
89-90:⚠️ Potential issueEnsure consistent type annotations between parameter and class attribute
The
memosparameter is typed aslist[str]here, but needs to be consistent with the class attribute type and how it's used in the code.- self, pattern: Pattern, memos: list[str], options: OptionsTab, operation: Operation + self, pattern: Pattern, memos: list[int], options: OptionsTab, operation: Operationsrc/main/python/main/ayab/engine/pattern.py (2)
37-37:⚠️ Potential issueFix type annotation for __memos attribute
There's a type mismatch. The
memosparameter should be typed aslist[int]but the__memosattribute is typed aslist[str], causing a pipeline failure.- self.__memos : list[str] = memos + self.__memos : list[int] = memos
33-33:⚠️ Potential issueEnsure consistent type annotations between files
The
memosparameter is typed aslist[str]here, butControlclass expects it to belist[int], causing pipeline failures. Ensure consistent typing across the codebase.- def __init__(self, image: Image.Image, memos: list[str], config: OptionsTab, num_colors: int = 2): + def __init__(self, image: Image.Image, memos: list[int], config: OptionsTab, num_colors: int = 2):
🧹 Nitpick comments (1)
src/main/python/main/ayab/engine/control.py (1)
237-240: Improve exception handling using contextlib.suppressInstead of using a bare
exceptclause, which can mask important errors, usecontextlib.suppressfor cleaner code.- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + from contextlib import suppress + with suppress(IndexError): + msg = msg + " memo: " + str(self.memos[self.pat_row])🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/python/main/ayab/engine/control.py(4 hunks)src/main/python/main/ayab/engine/pattern.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/main/python/main/ayab/engine/pattern.py (1)
src/main/python/main/ayab/engine/options.py (2)
OptionsTab(59-252)Alignment(255-266)
src/main/python/main/ayab/engine/control.py (2)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)Pattern(32-207)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
🪛 GitHub Actions: Python Tests
src/main/python/main/ayab/engine/control.py
[error] 277-277: Incompatible types in assignment (expression has type 'int', variable has type 'str') [assignment]
🔇 Additional comments (1)
src/main/python/main/ayab/engine/pattern.py (1)
38-39: Good practice: Explicit type annotationsAdding explicit type annotations for class attributes improves code readability and helps with static type checking. This is a good practice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/python/main/ayab/knitprogress.py (1)
277-285:⚠️ Potential issueFix type mismatches in the memo display method.
The
__memomethod has several type-related issues that need to be fixed:
- While the parameter type is now correctly set as
str, the implementation still assumes string behavior- The function should handle various data types more robustly
def __memo( self, memo: str, ) -> QTableWidgetItem: cell = QTableWidgetItem() cell.setBackground(QBrush(QColor(f"#{self.smoke:06x}"))) - if len(memo) > 0 and memo != "0": - cell.setText(memo) + if memo and memo != "0": + cell.setText(str(memo)) return cellAdditionally, consider adding a brief docstring explaining the purpose of this method:
def __memo( self, memo: str, ) -> QTableWidgetItem: + """ + Create a table cell for displaying memo information. + + Args: + memo: The memo string to display + + Returns: + A QTableWidgetItem configured to display the memo + """ cell = QTableWidgetItem() cell.setBackground(QBrush(QColor(f"#{self.smoke:06x}"))) if memo and memo != "0": cell.setText(str(memo)) return cell
🧹 Nitpick comments (2)
src/main/python/main/ayab/knitprogress.py (2)
39-39: Remove completed TODO comment.This TODO comment can now be removed since the memo information display feature has been implemented in this PR.
-# TODO (TP): display memo information
177-180: Consider adding a tooltip to explain the "M" header.While using "M" as the header for the memo column is space-efficient, it might not be immediately clear to users what it represents. Consider adding a tooltip to provide more context.
header = QTableWidgetItem("M") header.setForeground(QBrush(QColor(f"#{self.grey:06x}"))) + header.setToolTip("Memo information")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/knitprogress.py(8 hunks)
🔇 Additional comments (5)
src/main/python/main/ayab/knitprogress.py (5)
49-52: LGTM: Updated color constants and added new ones.The color constants are appropriately defined with the new
greyandsmokecolors for the memo display functionality.
84-84: LGTM: Smart decision to conditionally show memo column.Good approach to show the memo column only when there are memos present in the scene.
95-95: LGTM: UI state check updated for memo changes.Added check for memo status changes to trigger a UI refresh when needed.
143-144: LGTM: Conditionally add memo column to display.The implementation correctly adds the memo column only when memos are present.
175-175: LGTM: Proper needle index adjustment.The code correctly adjusts the needle index calculation when the memo column is present to maintain proper needle numbering.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/python/main/ayab/engine/control.py (1)
274-277:⚠️ Potential issueFix status memo type handling and improve error handling.
There's a type inconsistency and the bare
exceptshould be avoided as it catches all exceptions, potentially hiding unexpected errors.- try: - self.status.memo = self.memos[self.pat_row] - except: - self.status.memo = "0" + try: + self.status.memo = str(self.memos[self.pat_row]) + except IndexError: + self.status.memo = "0" + except Exception as e: + self.logger.warning(f"Error setting memo status: {str(e)}") + self.status.memo = "0"This ensures the memo is always converted to a string type before assignment and provides more specific error handling.
🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
🧹 Nitpick comments (2)
src/main/python/main/ayab/engine/control.py (2)
237-240: Improve error handling for memo logging.The current implementation uses a bare
exceptwhich catches all exceptions, potentially hiding unexpected errors. Additionally, it silently passes, which could make debugging difficult.- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + try: + msg = msg + " memo: " + str(self.memos[self.pat_row]) + except IndexError: + msg = msg + " memo: <none>" + except Exception as e: + self.logger.debug(f"Error retrieving memo for logging: {str(e)}")Alternatively, use contextlib.suppress as suggested by the static analysis:
- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + from contextlib import suppress + with suppress(Exception): + msg = msg + " memo: " + str(self.memos[self.pat_row])🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
237-240: Consider refactoring memo access to reduce code duplication.Both try-except blocks for memo access have similar logic but handle errors differently. A helper method would make this more consistent and maintainable.
+ def _get_memo(self, row_index: int, default_value: str = "0") -> str: + """Get the memo for the specified row index with error handling.""" + try: + return str(self.memos[row_index]) + except IndexError: + return default_value + except Exception as e: + self.logger.warning(f"Error accessing memo: {str(e)}") + return default_value +Then you could use it in both places:
- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + memo = self._get_memo(self.pat_row, "<none>") + msg = msg + " memo: " + memoAnd:
- try: - self.status.memo = self.memos[self.pat_row] - except: - self.status.memo = "0" + self.status.memo = self._get_memo(self.pat_row)Also applies to: 274-277
🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/engine/control.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
🔇 Additional comments (3)
src/main/python/main/ayab/engine/control.py (3)
64-64: Fix type annotation for memos attribute.There's a type inconsistency in the PR. The
memosattribute is defined aslist[str]here, but in a previous review comment, it was suggested to belist[int]. Based on line 277 where a string value "0" is assigned toself.status.memo, it appears that the type should indeed belist[str].
89-90: Good implementation of the memos parameter.The parameter is correctly added to the method signature and properly assigned to the instance variable. This matches the PR objectives of propagating memo information through the control flow.
Also applies to: 97-97
237-240:❓ Verification inconclusive
Verify that the memo implementation is robust for future data formats.
Based on the PR comments, there was a discussion about using JSON for memo data to improve flexibility. While the current implementation uses simple strings, consider whether this approach will be robust enough for future requirements.
If you plan to extend memo functionality in the future, a more structured format like JSON might be beneficial. You could implement this more robustly now, or document your decision to keep it simple with a comment explaining the tradeoffs.
Also applies to: 274-277
🏁 Script executed:
#!/bin/bash # Check how memo data is extracted from PNG files to understand the format rg -A 5 "extract.*memo" --glob "*.py"Length of output: 441
Action Required: Revisit Memo Handling for Future Scalability
The current implementation in
src/main/python/main/ayab/engine/control.py(lines 237–240 and 274–277) simply concatenates a memo string using a try/except block. Although this works now, a recent discussion suggested using a structured format like JSON for memo data to better support future requirements. Notably, a related TODO insrc/main/python/main/ayab/pattern_import.pyhighlights that memo extraction still needs attention.
- Consider: Whether a JSON-based approach or a similarly structured format might be more future-proof.
- Alternatively: Document the design decision for using simple strings by adding an inline comment, specifying the tradeoffs involved.
- Note: Evaluate if the current try/except block might hide errors during memo processing, which could become problematic as the memo format evolves.
🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/python/main/ayab/image.py (2)
59-59:⚠️ Potential issueFix type annotation for memos list.
There's a type mismatch across the codebase. In this file,
memosis typed aslist[str], but in other files it's expected to belist[int]. Pipeline failures confirm this inconsistency.- self.memos: list[str] = [] + self.memos: list[int] = []
117-131: 🛠️ Refactor suggestionImprove error handling in memo parsing.
The implementation has potential issues with error handling:
- Using a bare
exceptblock (line 127) can catch unexpected exceptions and mask errors.- The code would benefit from more robust error handling with specific error types.
- The logging is helpful, but could be more specific about what failed.
if suffix == ".png": # check metadata for memo information self.image.load() if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information - for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + try: + for i in range(len(comment) - 5): + char = comment[i + 5] + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}") # report metadata logging.info("File metadata Comment tag: " + comment) logging.info("File memo information: " + str(self.memos))🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/python/main/ayab/ayab.py(2 hunks)src/main/python/main/ayab/image.py(8 hunks)src/main/python/main/ayab/transforms.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
src/main/python/main/ayab/image.py (1)
src/main/python/main/ayab/transforms.py (10)
ImageTransform(29-131)Mirrors(134-147)invert(48-54)repeat(65-79)stretch(112-123)reflect(82-109)hflip(57-58)vflip(61-62)rotate_left(38-40)rotate_right(43-45)
src/main/python/main/ayab/transforms.py (1)
src/main/python/main/ayab/image.py (3)
hflip(174-175)repeat(139-152)vflip(177-178)
src/main/python/main/ayab/ayab.py (1)
src/main/python/main/ayab/transforms.py (1)
ImageTransform(29-131)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
src/main/python/main/ayab/transforms.py
99-99: Undefined name InageTransform
(F821)
🪛 GitHub Actions: Python Tests
src/main/python/main/ayab/transforms.py
[error] 99-99: F821 undefined name 'InageTransform'
🔇 Additional comments (8)
src/main/python/main/ayab/transforms.py (2)
29-29: Class renaming completed correctly.The class has been successfully renamed from
TransformtoImageTransform. This is a good change for clarity as it better represents the class's purpose of transforming images.
100-100: Method calls updated correctly.The method calls have been properly updated to use the new class name
ImageTransform. This ensures consistency across the codebase.Also applies to: 105-106
src/main/python/main/ayab/image.py (4)
30-30: Import statement updated correctly.The import statement has been properly updated to use
ImageTransforminstead ofTransform, maintaining consistency with the class renaming.
106-106: Proper initialization of memos list.The code correctly resets the
memoslist at the beginning of the__openmethod, ensuring there's no leftover data from previous operations.
137-137: Method calls updated correctly.All the method calls have been properly updated to use
ImageTransforminstead ofTransform, maintaining consistency with the class renaming.Also applies to: 152-152, 167-167, 172-172, 175-175, 178-178, 181-181, 184-184
92-97: Improved error messages in display_blocking_popup calls.The removal of "FIXME" comments from the display_blocking_popup calls improves the code quality. Error messages are now cleaner and more user-friendly.
src/main/python/main/ayab/ayab.py (2)
35-35: Import statement updated correctly.The import statement has been properly updated to use
ImageTransforminstead ofTransform, maintaining consistency with the class renaming.
121-121: Comment and code updated correctly.The comment and code have been properly updated to reference
ImageTransforminstead ofTransform, maintaining consistency with the class renaming.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/python/main/ayab/image.py (1)
59-59:⚠️ Potential issueFix type annotation for memos list
There's a type mismatch across the codebase. In this file,
memosis typed aslist[str], but in other files it's expected to belist[int]based on thePatternclass's memos accessor method. This inconsistency could lead to type errors or unexpected behavior.- self.memos: list[str] = [] + self.memos: list[int] = []
🧹 Nitpick comments (1)
src/main/python/main/ayab/image.py (1)
117-131: Consider using JSON format for memo dataBased on PR discussions, using JSON would provide more flexibility for future extensions (versioning, carriage information, colors, etc.) and simplify the parsing process.
if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): - # update memo information - for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + # Parse JSON data after "AYAB:" prefix + try: + import json + memo_data = json.loads(comment[5:]) + if "memos" in memo_data and isinstance(memo_data["memos"], list): + self.memos = [int(m) for m in memo_data["memos"] if isinstance(m, (int, str)) and (isinstance(m, int) or m.isdigit())] + except json.JSONDecodeError as e: + logging.error(f"Invalid JSON in memo data: {str(e)}") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}")🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/python/main/ayab/ayab.py(2 hunks)src/main/python/main/ayab/image.py(8 hunks)src/main/python/main/ayab/transforms.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/python/main/ayab/transforms.py
- src/main/python/main/ayab/ayab.py
🧰 Additional context used
🧬 Code Definitions (1)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/transforms.py (10)
ImageTransform(29-131)Mirrors(134-147)invert(48-54)repeat(65-79)stretch(112-123)reflect(82-109)hflip(57-58)vflip(61-62)rotate_left(38-40)rotate_right(43-45)src/main/python/main/ayab/engine/pattern.py (1)
memos(203-204)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
🔇 Additional comments (3)
src/main/python/main/ayab/image.py (3)
30-30: Transform class changes look goodThe consistent update from
TransformtoImageTransformacross all transform method calls is properly implemented. The code now uses theImageTransformclass which extendsImage.Imageand provides static methods for image transformations.Also applies to: 137-137, 152-152, 167-167, 172-172, 175-175, 178-178, 181-181, 184-184
92-92: Cleanup of error message blocksThe removal of FIXME comments from the display_blocking_popup calls is a good cleanup.
Also applies to: 97-97
106-106: Memo reset properly implementedClearing the memos list at the beginning of the
__openmethod ensures that memo information is properly refreshed when loading a new image.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/python/main/ayab/image.py (2)
59-59:⚠️ Potential issueFix type annotation for memos list
There's a type inconsistency between this declaration and how memos are used elsewhere in the codebase. In this file,
memosis typed aslist[str], but other parts of the codebase expect it to belist[int].- self.memos: list[str] = [] + self.memos: list[int] = []
124-128:⚠️ Potential issueImprove memo parsing with proper validation and error handling
The current implementation has several issues:
- Using a bare
exceptclause is considered poor practice- There's inconsistency between storing string values but handling them as integers elsewhere
- No validation is performed on the input characters
- for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + memo_data = comment[5:] + try: + for char in memo_data: + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data, skipping") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}")🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
🧹 Nitpick comments (3)
src/main/python/main/ayab/image.py (3)
183-184: Consider reversing memos instead of clearing them on vertical flipIf memos correspond to rows in the image, a vertical flip should reverse the memos list rather than clearing it, which would preserve the memo information in the correct order for the flipped image.
self.apply_transform(ImageTransform.vflip) - self.memos = [] + self.memos = self.memos[::-1] # Reverse the list
153-154: Consider horizontal repetition for memosWhen repeating the image, you only multiply the memos list by the vertical repeat factor. If memos correspond to rows, this makes sense, but consider also including a comment to explain this behavior for clarity.
self.apply_transform(ImageTransform.repeat, v[0], h[0]) - self.memos = self.memos * v[0] + # Multiply memos by vertical repeat factor only since memos correspond to rows + self.memos = self.memos * v[0]
117-131: Consider using JSON format for memo data as suggested in PR commentsAs discussed in the PR comments, using JSON for memo data would provide more flexibility and simplify parsing. The current string-based approach limits you to single characters and requires more complex parsing logic.
comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information - for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + try: + import json + # Extract JSON data after "AYAB:" + json_data = comment[5:] + memo_data = json.loads(json_data) + if isinstance(memo_data, list): + self.memos = [int(m) for m in memo_data if isinstance(m, (int, str)) and str(m).isdigit()] + else: + logging.warning(f"Invalid memo data format: {memo_data}") + except json.JSONDecodeError as e: + logging.error(f"Error parsing JSON memo data: {str(e)}") + # Fall back to old format for backward compatibility + for char in comment[5:]: + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data, skipping") + except Exception as e: + logging.error(f"Error processing memo data: {str(e)}")Do you think using JSON would be a better approach for storing memo data? This would align with the suggestion from contributor jpcornil-git in the PR comments.
🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/image.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/main/python/main/ayab/image.py (1)
src/main/python/main/ayab/transforms.py (10)
ImageTransform(29-131)Mirrors(134-147)invert(48-54)repeat(65-79)stretch(112-123)reflect(82-109)hflip(57-58)vflip(61-62)rotate_left(38-40)rotate_right(43-45)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
|
Treatment of Image Transforms:
|
|
I don't know how to freeze the Memo column. @X-sam ? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/python/main/ayab/engine/control.py (1)
274-277: 🛠️ Refactor suggestionFix error handling in status update
The code uses a bare
exceptclause which can mask unexpected errors. Additionally, specify the type of exceptions you expect.- try: - self.status.memo = self.memos[self.pat_row] - except: - self.status.memo = "0" + try: + self.status.memo = self.memos[self.pat_row] + except IndexError: + self.status.memo = "0" + except Exception as e: + self.logger.warning(f"Error setting memo status: {str(e)}") + self.status.memo = "0"🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
🧹 Nitpick comments (3)
src/main/python/main/ayab/image.py (3)
169-170: Document memo behavior for image transformationsThe
stretchtransform clears memos whilerepeatmultiplies them. Consider adding a comment explaining why these transformations affect memo data differently.self.apply_transform(ImageTransform.stretch, v[0], h[0]) - self.memos = [] + # Clear memos as stretching distorts the relationship between rows and memo data + self.memos = []
175-176: Consider consistent documentation for memo behaviorMultiple transformations clear the memo data without explanation. Add consistent documentation to explain the memo handling policy for each transform.
self.apply_transform(ImageTransform.reflect, m.mirrors) - self.memos = [] + # Clear memos as reflection changes the structure of the image rows + self.memos = []Apply similar comments to the other transforms that clear memo data.
Also applies to: 183-184, 187-188, 191-192
124-128: Consider implementing reviewer suggestion for JSON memo formatAs discussed in the PR comments, using JSON for memo data would provide more flexibility and extensibility. This would allow storing structured data rather than just single characters.
Example implementation for JSON-based memos:
import json # When parsing the comment: if comment.startswith("AYAB:"): try: json_data = comment[5:] memo_dict = json.loads(json_data) # This allows for future expansion with fields like "version", "colors", etc. self.memos = memo_dict.get("memos", []) logging.info(f"Parsed JSON memo data: {memo_dict}") except json.JSONDecodeError: # Fallback for backward compatibility logging.info("Using legacy memo format") self.memos = [char for char in comment[5:]] except Exception as e: logging.error(f"Error parsing memo data: {str(e)}") self.memos = []This would support both the current simple format and a more flexible JSON format for future enhancements.
🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/python/main/ayab/ayab.py(2 hunks)src/main/python/main/ayab/engine/control.py(4 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/pattern.py(2 hunks)src/main/python/main/ayab/engine/status.py(3 hunks)src/main/python/main/ayab/gui_fsm.py(2 hunks)src/main/python/main/ayab/image.py(8 hunks)src/main/python/main/ayab/knitprogress.py(8 hunks)src/main/python/main/ayab/pattern_import.py(1 hunks)src/main/python/main/ayab/tests/test_control.py(7 hunks)src/main/python/main/ayab/transforms.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/python/main/ayab/pattern_import.py
- src/main/python/main/ayab/engine/status.py
- src/main/python/main/ayab/gui_fsm.py
- src/main/python/main/ayab/transforms.py
- src/main/python/main/ayab/knitprogress.py
- src/main/python/main/ayab/engine/pattern.py
- src/main/python/main/ayab/ayab.py
- src/main/python/main/ayab/tests/test_control.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/python/main/ayab/engine/control.py (3)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)Pattern(32-207)src/main/python/main/ayab/engine/options.py (1)
OptionsTab(59-252)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
src/main/python/main/ayab/image.py (2)
src/main/python/main/ayab/transforms.py (10)
ImageTransform(29-131)Mirrors(134-147)invert(48-54)repeat(65-79)stretch(112-123)reflect(82-109)hflip(57-58)vflip(61-62)rotate_left(38-40)rotate_right(43-45)src/main/python/main/ayab/engine/pattern.py (1)
memos(203-204)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Create and upload Linux AppImage release
- GitHub Check: Create and upload Windows build
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Linux AppImage release
- GitHub Check: Create and upload Windows build
🔇 Additional comments (15)
src/main/python/main/ayab/engine/control.py (2)
64-64: Fix type annotation for memos attributeThe memos attribute should be typed as
list[str]to match the implementation. This is consistent with how memo data is stored in theAyabImageclass (line 59) and used throughout the codebase.
89-90: LGTM: Updated method signature to include memo informationThe
startmethod now accepts amemosparameter, maintaining consistency with the memo implementation across the codebase.Also applies to: 97-97
src/main/python/main/ayab/engine/engine.py (7)
38-38: LGTM: Updated import to use AyabImageThe import statement now correctly imports AyabImage, which is necessary for accessing the memo data.
54-54: LGTM: Added memos attribute to Engine classThe
memosattribute is correctly added to the Engine class and initialized in the constructor.
71-71: LGTM: Properly initialized memos listThe
memosattribute is correctly initialized as an empty list in the constructor.
122-122: LGTM: Updated method signature to use AyabImageThe
knit_configmethod now correctly accepts anAyabImageinstead of a PILImage.Image, allowing access to the memo data.
131-131: LGTM: Updated image processing to use AyabImageThe code correctly accesses the
imageattribute of theAyabImageobject.
135-136: LGTM: Proper assignment of pattern and memosThe code correctly passes the image and memos to the
Patternconstructor and assignsim.memostoself.memos.
173-173: LGTM: Updated control.start call to include memosThe
runmethod now correctly passesself.memostoself.control.start.src/main/python/main/ayab/image.py (6)
30-30: LGTM: Updated import to use ImageTransformThe import statement now correctly uses
ImageTransforminstead ofTransform. This is consistent with the actual class name used in the transforms module.
59-59: LGTM: Added memos attribute to AyabImage classThe
memosattribute is correctly initialized as an empty list of strings.
106-106: LGTM: Properly initialize memos listThe
memoslist is correctly initialized at the beginning of the__openmethod.
137-138: LGTM: Updated transform call and maintained memo dataThe code correctly calls
ImageTransform.invertand preserves memo data, which is appropriate for this transform.
153-154: LGTM: Updated transform call with memo handlingThe code correctly multiplies the memo list by the vertical repeat factor, ensuring alignment with the expanded image rows.
179-180: LGTM: Maintained memo data for horizontal flipThe code correctly preserves memo data for horizontal flipping, which is appropriate since it doesn't affect row organization.
src/main/python/main/ayab/image.py
Outdated
| if suffix == ".png": | ||
| # check metadata for memo information | ||
| self.image.load() | ||
| if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: | ||
| comment = str(self.image.info["Comment"]) | ||
| if comment.startswith("AYAB:"): | ||
| # update memo information | ||
| for i in range(len(comment) - 5): | ||
| try: | ||
| self.memos.append(comment[i + 5]) | ||
| except: | ||
| self.memos.append('0') | ||
| # report metadata | ||
| logging.info("File metadata Comment tag: " + comment) | ||
| logging.info("File memo information: " + str(self.memos)) | ||
| self.image = self.image.convert("RGBA") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve memo extraction with proper validation and error handling
The current implementation has several issues:
- It uses a bare
exceptclause which can mask unexpected errors - There's no validation that the characters after "AYAB:" are valid memo data
- The error handling simply defaults to '0' without logging the issue
if suffix == ".png":
# check metadata for memo information
self.image.load()
if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0:
comment = str(self.image.info["Comment"])
if comment.startswith("AYAB:"):
# update memo information
- for i in range(len(comment) - 5):
- try:
- self.memos.append(comment[i + 5])
- except:
- self.memos.append('0')
+ memo_data = comment[5:]
+ logging.info(f"Extracting memo data: '{memo_data}'")
+ for char in memo_data:
+ self.memos.append(char)
# report metadata
logging.info("File metadata Comment tag: " + comment)
logging.info("File memo information: " + str(self.memos))Consider the JSON format suggested in PR comments for more flexibility:
import json
# Within the if comment.startswith("AYAB:") block:
try:
json_data = comment[5:]
memo_dict = json.loads(json_data)
self.memos = memo_dict.get("memos", [])
logging.info(f"Parsed JSON memo data: {self.memos}")
except json.JSONDecodeError as e:
logging.warning(f"Invalid JSON in memo data: {e}")
# Fall back to character-by-character parsing
self.memos = [char for char in comment[5:]]
except Exception as e:
logging.error(f"Error parsing memo data: {str(e)}")🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare except
(E722)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/main/python/main/ayab/engine/control.py (3)
64-64:⚠️ Potential issueFix type annotation for the memos attribute.
The memos attribute is defined with the type
list[str]here, but there's a type mismatch in the codebase. Based on past reviews, there appear to be expectations that this should belist[int]in some places.- memos: list[str] + memos: list[int]
237-240: 🛠️ Refactor suggestionImprove error handling for memo logging.
The bare
exceptclause is considered poor practice as it catches all exceptions, including KeyboardInterrupt and SystemExit. Additionally, the silent pass can mask unexpected errors.- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + from contextlib import suppress + with suppress(IndexError): + msg = msg + " memo: " + str(self.memos[self.pat_row])Alternatively, you could use more specific exception handling:
- try: - msg = msg + " memo: " + str(self.memos[self.pat_row]) - except: - pass + try: + msg = msg + " memo: " + str(self.memos[self.pat_row]) + except IndexError: + # Handle the case when pat_row is out of bounds + pass + except Exception as e: + self.logger.debug(f"Error logging memo: {str(e)}")🧰 Tools
🪛 Ruff (0.8.2)
237-240: Use
contextlib.suppress(Exception)instead oftry-except-passReplace with
contextlib.suppress(Exception)(SIM105)
239-239: Do not use bare
except(E722)
274-277:⚠️ Potential issueFix status memo type handling.
There's a type mismatch here:
self.status.memois expected to be a string, but ifself.memosis intended to be a list of integers (as suggested by other parts of the codebase), you should explicitly convert the value to a string. Also, the bareexceptshould be replaced with specific exception handling.- try: - self.status.memo = self.memos[self.pat_row] - except: - self.status.memo = "0" + try: + self.status.memo = str(self.memos[self.pat_row]) + except IndexError: + self.status.memo = "0" + except Exception as e: + self.logger.warning(f"Error setting memo status: {str(e)}") + self.status.memo = "0"🧰 Tools
🪛 Ruff (0.8.2)
276-276: Do not use bare
except(E722)
src/main/python/main/ayab/image.py (2)
59-59:⚠️ Potential issueFix type annotation for memos list.
There's a type mismatch across the codebase. In this file,
memosis typed aslist[str], but in other files, it might be expected to belist[int]. This inconsistency could lead to type errors.- self.memos: list[str] = [] + self.memos: list[int] = []
124-128: 🛠️ Refactor suggestionImprove memo parsing with proper validation and error handling.
The current implementation has several issues:
- Using a bare
exceptclause is considered poor practice- There's no validation that the characters are valid memo data
- The error handling simply defaults to '0' without logging the issue
- for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + memo_data = comment[5:] + try: + for char in memo_data: + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data, skipping") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}")🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
🧹 Nitpick comments (2)
src/main/python/main/ayab/image.py (2)
117-132: Consider using JSON format for memo data.Based on the PR discussion, jpcornil-git suggested using a JSON format for storing memo data, which would provide greater flexibility for future extensions. This would allow for adding more data types like versioning, carriage information, and colors.
Consider implementing a JSON-based approach:
if suffix == ".png": # check metadata for memo information self.image.load() if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): - # update memo information - for i in range(len(comment) - 5): - try: - self.memos.append(comment[i + 5]) - except: - self.memos.append('0') + # Parse JSON data + import json + try: + json_data = comment[5:] + memo_dict = json.loads(json_data) + if "memos" in memo_dict and isinstance(memo_dict["memos"], list): + self.memos = [int(m) if isinstance(m, (int, str)) and str(m).isdigit() else 0 + for m in memo_dict["memos"]] + logging.info(f"Parsed JSON memo data: {self.memos}") + except json.JSONDecodeError as e: + logging.warning(f"Invalid JSON in memo data: {e}") + # Fall back to character-by-character parsing + self.memos = [int(char) if char.isdigit() else 0 for char in comment[5:]] + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}") # report metadata logging.info("File metadata Comment tag: " + comment) logging.info("File memo information: " + str(self.memos))This approach provides:
- Flexibility to add more data in the future
- Proper error handling with specific exceptions
- A fallback mechanism for backward compatibility
🧰 Tools
🪛 Ruff (0.8.2)
127-127: Do not use bare
except(E722)
169-170: Communicate memo clearing to users.When performing operations that clear memo data (stretch, reflect, vflip, rotate_left, rotate_right), consider logging this action and potentially informing the user that memo information will be lost.
self.apply_transform(ImageTransform.stretch, v[0], h[0]) + if self.memos: + logging.info("Clearing memo information after stretch operation") self.memos = []Apply similar changes to the other methods that clear memo data.
Also applies to: 175-176, 183-184, 187-188, 191-192
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/python/main/ayab/ayab.py(2 hunks)src/main/python/main/ayab/engine/control.py(4 hunks)src/main/python/main/ayab/engine/engine.py(6 hunks)src/main/python/main/ayab/engine/pattern.py(2 hunks)src/main/python/main/ayab/engine/status.py(3 hunks)src/main/python/main/ayab/gui_fsm.py(2 hunks)src/main/python/main/ayab/image.py(8 hunks)src/main/python/main/ayab/knitprogress.py(8 hunks)src/main/python/main/ayab/pattern_import.py(1 hunks)src/main/python/main/ayab/tests/test_control.py(7 hunks)src/main/python/main/ayab/transforms.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/python/main/ayab/pattern_import.py
- src/main/python/main/ayab/gui_fsm.py
- src/main/python/main/ayab/transforms.py
- src/main/python/main/ayab/ayab.py
- src/main/python/main/ayab/engine/status.py
- src/main/python/main/ayab/tests/test_control.py
- src/main/python/main/ayab/engine/pattern.py
- src/main/python/main/ayab/engine/engine.py
- src/main/python/main/ayab/knitprogress.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/python/main/ayab/engine/control.py (2)
src/main/python/main/ayab/engine/pattern.py (2)
memos(203-204)Pattern(32-207)src/main/python/main/ayab/engine/engine_fsm.py (1)
Operation(39-41)
🪛 Ruff (0.8.2)
src/main/python/main/ayab/engine/control.py
237-240: Use contextlib.suppress(Exception) instead of try-except-pass
Replace with contextlib.suppress(Exception)
(SIM105)
239-239: Do not use bare except
(E722)
276-276: Do not use bare except
(E722)
src/main/python/main/ayab/image.py
127-127: Do not use bare except
(E722)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Linux AppImage release
- GitHub Check: run-pytest
- GitHub Check: Create and upload Windows build
- GitHub Check: Create and upload Windows build
- GitHub Check: Create and upload Linux AppImage release
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: run-pytest
🔇 Additional comments (1)
src/main/python/main/ayab/image.py (1)
153-154: Verify memo handling in repeat operations.The implementation assumes that memos correspond to rows in the image and simply multiplies the list by the vertical repeat count. Ensure this is the correct behavior, especially if memos contain row-specific information.
If memos contain more complex data or have specific meanings per row, consider more sophisticated handling, such as:
- self.memos = self.memos * v[0] + # Create new memo list with appropriate values for each repeated row + original_memos = self.memos.copy() + self.memos = [] + for _ in range(v[0]): + self.memos.extend(original_memos) + logging.info(f"Repeated memos {v[0]} times, new count: {len(self.memos)}")
|
What's the status of this PR? |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/python/main/ayab/image.py (1)
203-204: 🛠️ Refactor suggestionAdd error handling to the transform application
The previous version of this method likely had try-except error handling which has been removed. Consider adding appropriate error handling to prevent crashes if a transform operation fails.
def apply_transform( self, transform: Callable[..., Image.Image], *args: tuple[int, int] | list[int] | int, ) -> None: """Executes an image transform specified by function and args.""" - self.image = transform(self.image, args) + try: + self.image = transform(self.image, args) + except Exception as e: + display_blocking_popup( + QCoreApplication.translate("Image", "Error applying transform"), "error" + ) + logging.error(f"Error in transform {transform.__name__}: {str(e)}") + return
♻️ Duplicate comments (2)
src/main/python/main/ayab/image.py (2)
59-59:⚠️ Potential issueFix type annotation for memos list
There's a type mismatch across the codebase. In this file,
memosis typed aslist[str], but in other files it's expected to belist[int]. This inconsistency can lead to runtime errors.- self.memos: list[str] = [] + self.memos: list[int] = []
124-126:⚠️ Potential issueImprove memo parsing with proper validation and error handling
The current implementation lacks validation for the memo characters and doesn't handle potential errors. This could cause issues if the memo format is unexpected.
- for i in range(len(comment) - 5): - self.memos.append(comment[i + 5]) + memo_data = comment[5:] + try: + for char in memo_data: + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data, skipping") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}")
🧹 Nitpick comments (1)
src/main/python/main/ayab/image.py (1)
117-128: Consider JSON format for better extensibilityAs discussed in the PR comments, using JSON for memo data would provide better extensibility and simplify parsing. It would allow for additional metadata and remove the limitation of single-digit numbers.
if "Comment" in self.image.info and len(str(self.image.info["Comment"])) > 0: comment = str(self.image.info["Comment"]) if comment.startswith("AYAB:"): # update memo information - for i in range(len(comment) - 5): - self.memos.append(comment[i + 5]) + try: + import json + json_data = comment[5:] + memo_dict = json.loads(json_data) + self.memos = memo_dict.get("memos", []) + logging.info(f"Parsed JSON memo data: {self.memos}") + except json.JSONDecodeError: + # Fall back to character-by-character parsing for backward compatibility + memo_data = comment[5:] + self.memos = [] + for char in memo_data: + if char.isdigit(): + self.memos.append(int(char)) + else: + logging.warning(f"Non-digit character '{char}' found in memo data, skipping") + except Exception as e: + logging.error(f"Error parsing memo data: {str(e)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/python/main/ayab/image.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Create and upload Linux AppImage release
- GitHub Check: Create and upload Windows build
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Mac OSX build
- GitHub Check: Create and upload Windows build
- GitHub Check: Create and upload Linux AppImage release
🔇 Additional comments (1)
src/main/python/main/ayab/image.py (1)
150-151: Verify memo multiplication in repeat functionWhen repeating an image vertically, the memos are multiplied by the vertical repeat count. Ensure this is the desired behavior - will users expect to see the same memo value repeated for all repeated rows, or should the memo values only apply to the original rows?
Consider validating this behavior with users to confirm it matches their expectations.
| self.apply_transform(ImageTransform.stretch, v[0], h[0]) | ||
| self.memos = [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider preserving memo data for safe transforms
The current implementation clears memos for several transforms (stretch, reflect, vflip, rotate_left, rotate_right). According to the PR discussion, some of these might be safe in specific circumstances.
For user experience consistency, consider adding a warning dialog when a transform would clear memo data, giving users the option to proceed or cancel.
def _clear_memos_with_warning(self, transform_name):
"""Display warning and clear memos if user confirms."""
result = display_warning_dialog(
f"The {transform_name} transform will clear memo data. Proceed?",
buttons=["Yes", "No"]
)
if result == "Yes":
self.memos = []
return True
return False
# Then in transform methods like stretch:
def stretch(self):
# ... existing code ...
if self.memos and not self._clear_memos_with_warning("Stretch"):
return # User canceled
self.apply_transform(ImageTransform.stretch, v[0], h[0])Also applies to: 172-173, 180-181, 184-185, 188-189
|
|
Not impressed with the remaining CodeRabbit suggestions. |
Please comment and resolve them, so we can proceed with the testing step. But out of curiosity: #779 (comment) suggests improvements for handling the transforms where memo is about to be deleted, something you also mentioned in #779 (comment). So may be worth to follow the suggestion? |
| if n_cols < 4: | ||
| self.hideColumn(5) |
There was a problem hiding this comment.
Removing those two lines (which do look like dead code, as the number of columns is synchronized with the size of the columns array earlier, so column 5 can't exist if len(columns) < 4) makes the n_cols assignment above obsolete as well.
Addressing #776
Draft format described here: AllYarnsAreBeautiful/ayab-patterns#4 (comment)
Up-to-date testing builds for this branch can be found at: https://nightly.link/AllYarnsAreBeautiful/ayab-desktop/workflows/build-multi-os/memo
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style
Chores