Skip to content

GH-959 Move warp contents from language files. #1172

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

GH-959 Move warp contents from language files. #1172

wants to merge 5 commits into from
+564 −440

Conversation

@vLuckyyy
Copy link
Member

Fixes: GH-959

@vLuckyyy vLuckyyy requested a review from a team as a code owner September 19, 2025 21:21
gemini-code-assist[bot]
gemini-code-assist bot reviewed Sep 19, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request successfully refactors the warp inventory configuration from language files into a dedicated warp-inventory.yml, which is a significant improvement for maintainability. The adoption of a repository pattern with asynchronous, transactional operations is a solid architectural choice. However, I've identified a few critical issues, including a race condition and a bug where changes are not persisted, which could lead to data inconsistency. My review provides detailed feedback and suggestions to address these problems by better utilizing the new repository pattern.

CitralFlo
CitralFlo reviewed Sep 23, 2025
View reviewed changes
Copy link
Member

@CitralFlo CitralFlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Rollczi that the repository class it's complicated and could cause problems when maintaining the code in the future

Jakubk15
Jakubk15 approved these changes Sep 24, 2025
View reviewed changes
Copy link
Member

@Jakubk15 Jakubk15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test the changes and ensure the behavior of the inventories is proper

Rollczi
Rollczi requested changes Oct 26, 2025
View reviewed changes
Copy link
Member

@Rollczi Rollczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dobra jednak nie, te repo z completable feature trochę mnie trigeruje słabo ten kod wygląda dużo nam to komplikuje, nie możemy tego ładniej zrobić? np. removeWarp to już tam się dzieją dziwne rzeczy

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

Reviewers

@Rollczi Rollczi Rollczi requested changes

@igoyek igoyek igoyek approved these changes

@Jakubk15 Jakubk15 Jakubk15 approved these changes

@P1otrulla P1otrulla P1otrulla approved these changes

@CitralFlo CitralFlo CitralFlo approved these changes

@imDMK imDMK Awaiting requested review from imDMK

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move the warp GUI 100% to warps.yml (without messages)

8 participants

@vLuckyyy @Rollczi @igoyek @Jakubk15 @P1otrulla @CitralFlo @imDMK