Add basic support for mineral containers #111
Conversation
|
Hello! The CI seems to be failing, but there's a lot of warnings cluttering the view, so don't worry about that yet. I'll make a PR myself to fix the stuff mypy is complaining about, and when I'm done we can update your branch. In the meantime, could you add some tests for this new feature? |
AnthonyMichaelTDM
force-pushed
the
feat/mineral-containers
branch
from
3ce9ac6 to
b3bcb39
Compare
|
okay, I updated your branch but didn't change anything else. From my understanding you can have multiple of even the same kind of mineral container, how is that being handled? |
AnthonyMichaelTDM
added
the
Status: In Progress
|
Yes you can have multiple of each type of mineral. Since originally the cores just showed up as unknown, I just changed it to the mineral type. So it's just a list of all the cores/mineral containers. I didn't bother changing it to show like Before: |
|
Looks good, thanks for the confirmation. I'm good to review and merge this once you get the CI checks to pass |
|
This updates the tests and fixes the mypy error. Personally I think what I mentioned earlier about the mineral counts would feel appropriate in the overclock tree ( |
|
yeah, I have to agree. I think being in the sidebar is better |
|
So just completely remove it from the centre overclock tree, including the |
|
Honestly it's up to you, I didn't mean to change how things were rendered so don't take that it renders in the OC tree now as a preference of mine or anything. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds basic support for mineral containers by creating a new category and displaying them in the overclock tree with the format "Mineral: {name} ({guid})".
- Adds MINERAL_CONTAINERS category to the Category enum
- Updates the data structure to handle mineral containers separately from other overclocks
- Integrates mineral container display into the UI tree and unforged list
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/python/helpers/enums.py | Adds MINERAL_CONTAINERS category enum |
| src/main/python/helpers/datatypes.py | Refactors OcData to handle mineral containers with dedicated field and logic |
| src/main/python/core/view.py | Updates UI to display mineral containers in tree and unforged list |
| src/main/python/core/state_manager.py | Adds mineral container data reshaping logic |
| src/main/python/core/file_parser.py | Handles mineral container overclock creation with hardcoded values |
| guids.json | Adds GUID mappings for 6 mineral types |
| tests/*.py | Updates test expectations for new overclock counts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self.overclocks.append( | ||
| Overclock( | ||
| category=overclock_data.category, | ||
| dwarf=Dwarf.SCOUT, |
There was a problem hiding this comment.
Hardcoding Dwarf.SCOUT for mineral containers is inconsistent with the empty dwarf string used in state_manager.py. Consider using a consistent approach or adding a comment explaining why SCOUT is used here.
| dwarf=Dwarf.SCOUT, | |
| dwarf="", |
| category=overclock_data.category, | ||
| dwarf=Dwarf.SCOUT, | ||
| weapon="", | ||
| cost=Cost(credits=600), |
There was a problem hiding this comment.
The hardcoded cost of 600 credits for mineral containers should be documented or made configurable. Consider adding a constant or comment explaining this value.
| cost=Cost(credits=600), | |
| cost=Cost(credits=MINERAL_CONTAINER_CREDIT_COST), |
| for k, v in data[Category.MINERAL_CONTAINERS.value].items(): | ||
| new_data[k] = { | ||
| "category": Category.MINERAL_CONTAINERS, | ||
| "dwarf": "", |
There was a problem hiding this comment.
Using an empty string for dwarf is inconsistent with the Dwarf enum usage elsewhere. Consider using a specific enum value like Dwarf.NONE or None to be more explicit about the absence of a dwarf association.
| "dwarf": "", | |
| "dwarf": None, |
| ): | ||
| for category, data in oc_data.non_weapon.items(): | ||
| for category, data in oc_data.other.items(): | ||
| if category == Category.WEAPONS or category == Category.MINERAL_CONTAINERS: |
There was a problem hiding this comment.
[nitpick] This condition check to skip WEAPONS and MINERAL_CONTAINERS is fragile. Consider refactoring to make the separation of concerns more explicit, perhaps by using a set of categories to skip or checking for specific categories to include.
| if category == Category.WEAPONS or category == Category.MINERAL_CONTAINERS: | |
| SKIP_CATEGORIES = {Category.WEAPONS, Category.MINERAL_CONTAINERS} | |
| for category, data in oc_data.other.items(): | |
| if category in SKIP_CATEGORIES: |
2 participants
This PR's intention is to add basic support for seeing the mineral names for mineral containers.
I didn't really bother adding more support/features for mineral containers (like how many you have of a certain type).
It's really just to add the GUIDs of mineral containers, and displaying
Mineral: Bismor (C709D...)for them. If more should be done, or the current implementation is too 'hacky' let me know 🙂.