Skip to content

storage#72

Open
JoanJaraBosch wants to merge 10 commits intoTheDevOpsBlueprint:mainfrom
JoanJaraBosch:origin/65
Open

storage#72
JoanJaraBosch wants to merge 10 commits intoTheDevOpsBlueprint:mainfrom
JoanJaraBosch:origin/65

Conversation

@JoanJaraBosch
Copy link

@JoanJaraBosch JoanJaraBosch commented Oct 2, 2025

PR Checklist

  • Follows single-purpose principle
  • Tests pass locally
  • Documentation updated (if needed)

What does this PR do?

Related Issue

Closes #65

Type of change

  • Bug fix
  • New feature
  • Configuration
  • Documentation

@Valentin-v-Todorov
Copy link
Contributor

@JoanJaraBosch Please resolve the conflicts let me know when the PR is ready for review ✌️

@JoanJaraBosch
Copy link
Author

hi @Valentin-v-Todorov conflicts resolved

@Valentin-v-Todorov
Copy link
Contributor

Hello @JoanJaraBosch Thanks for implementing the archive functionality. The core features should working as expected, but there are some issues that need to be addressed before we can merge:

🔴 :
The cli.py file has extensive duplication (duplicate imports, duplicate command implementations, multiple if name == 'main' blocks)
Please resolve and remove all duplicate code sections

There is a bug in unarchive command. I think that currently creates a new task instead of restoring the original. Instead of storage.add_task(...), directly restore the task object to preserve its original ID and metadata.

The archived field was added to Task model but isn't being used. Since you're using separate storage (archived.json), please remove the archived field from the Task model to avoid confusion

🟡 :
Add edge case handling. Check if task is already archived before archiving. Handle ID conflicts when unarchiving (check if ID already exists in active tasks)

🟢 :
However, I think those are working well :

✓ tix archive command properly soft-deletes tasks
✓ tix archived command lists archived tasks
✓ Separate storage file for archived tasks
✓ Context-aware archiving

Once these issues are resolved, ping me to review. The core functionality looks good - we just need to clean up the code quality issues.

Overall good job ! ✌️

@JoanJaraBosch
Copy link
Author

hello @Valentin-v-Todorov I am a bit sleepy, but I think it is ok now. If not tell me

@Valentin-v-Todorov
Copy link
Contributor

Thank you @JoanJaraBosch for working on the archive functionality. I do appreciate it! While the core concept is good, there are still critical issues that will break the tool if merged.

File: tix/cli.py
The add command (lines 160-168) has incomplete link handling code that's cut off mid-implementation, causing syntax errors.
Multiple commands have severe code duplication with implementations appearing twice: search (lines 517-620), tags (lines 677-730), and report (lines 780-929). This creates duplicate function definitions that will break Python.
The unarchive command (line 1177) calls storage.save_task(task) which doesn't exist in TaskStorage. You need to use existing storage method.
Missing imports for ContextStorage and references to non-existent modules like .utils, .storage, and .context.

File: tix/models.py
The from_dict method has been rewritten using **data which breaks backward compatibility. Legacy tasks with unexpected fields will cause crashes. The safe field handling with data.get() and defaults has been removed.

Remove all duplicate command implementations. Each command should appear only once.
Fix the unarchive command to use existing storage methods: load tasks, append the restored task, and call save_tasks.
Restore the safe from_dict implementation using data.get() with appropriate defaults for backward compatibility.
Clean up imports and remove references to non-existent modules.

The ArchiveStorage class looks good and the archive concept is solid. However, the code duplication and broken implementations will cause the tool to fail completely. Please fix these issues and push an updated version. Once cleaned up, this will be a valuable addition to TIX.

@JoanJaraBosch
Copy link
Author

Hi @Valentin-v-Todorov tell me if it is okay now

@Valentin-v-Todorov
Copy link
Contributor

Valentin-v-Todorov commented Oct 9, 2025

Hey @JoanJaraBosch, thanks for the continued work on this. I can see you've made progress, but there are still some critical issues that will break the tool if merged.

File: tix/cli.py:

Line 1 has a broken import - from tix.utils import get_date but later at line 12 you import it again. You need to remove the duplicate import on line 1 and keep only the one at line 12.

Lines 414-435 have duplicate code for handling attachments and due dates in the edit command. The attachment handling code appears twice with the second block starting at line 464. You need to remove one of these duplicate blocks completely.

Lines 536-551 in the priority command have the same logic written twice - setting old_priority, updating the task, and printing the result. Remove the duplicate lines 546-551.

Lines 638-680 in the move command contain duplicate implementations. The logic for moving tasks is written twice with slightly different approaches. Keep only one implementation and remove the other.

Line 27 creates a global cli variable with cli = click.Group() but then line 31 redefines it as a function with the @click.group() decorator. This will cause a conflict. Remove line 27 completely.

The unarchive command at lines 1179-1198 looks correct now and properly uses storage.save_tasks() to restore tasks, which is good.

File: tix/models.py:

Lines 16-17 have links field defined twice. Remove the duplicate on line 17.
The is_global field addition on line 16 is fine, but it's not being used anywhere in the PR. If it's for future work, that's okay, but make sure to document it.

File: tix/storage/context_storage.py:

This stub implementation is acceptable but very minimal. It won't actually persist context data. For now this is fine since it prevents import errors, but the functionality is limited.

The archive functionality itself (ArchiveStorage class and the three archive commands) looks solid now. Once you clean up the duplicate code in cli.py and the duplicate field in models.py, this should be ready to merge. Please fix these issues and push an update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add task archiving instead of deletion

2 participants