Skip to content

Conversation

@MagicTheDev
Copy link
Collaborator

Copy link
Collaborator

@doluk doluk left a comment

Choose a reason for hiding this comment

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

review of the code, no testing.
Some minor changes for the calculation of upgrade/training times mostly

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure I like having a massive json with basically independent subjsons structures over the previous multiple jsons. This makes reviewing changes a lot harder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't find it harder to compare changes + just one file to check 👀

Honestly can see both ways but one file has conveniences

# keep the raw CSV files
self.KEEP_CSV = False
# keep the raw JSON files
self.KEEP_JSON = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those options should be at least accessible via kwargs in the constructor, in my opinion


self.TARGETS = []
self.supported_languages: list[str] = []
self.USED_TIDS = set()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should probably be marked as internal. or not even be a traditional set in the first place and instead be a property and calculated on the fly, when accessed?

"level": int(level),
"hitpoints": level_data.get("Hitpoints"),
"dps": level_data.get("DPS"),

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have the data to add here an ability property?

elif is_super_troop: #for super troops use the original troop's lab level'
original_troop = self.full_troop_data.get(super_troop_data["Original"])
required_lab_level = original_troop.get(level).get("LaboratoryLevel")
required_townhall = max_townhall_converter[required_lab_level]
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have the data to add an ability property? so for example super minions range shots are accessible

Copy link
Collaborator Author

@MagicTheDev MagicTheDev Nov 30, 2025

Choose a reason for hiding this comment

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

Abilities (even if there) are a tricky thing, I did add to heroes buttt the structure for those is far from consistent & I just leave them as raw JSON. Maybe an ability class needs to be made one day that can handle lm the different portal ability types (dps , heal, spawner, aura, etc)

self.realtime = realtime
self.raw_attribute = raw_attribute
self.correct_tags = correct_tags
self.load_game_data = load_game_data
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has no function anymore or?


class VillageType(ExtendedEnum):
home = "home"
builder_base = "builderBase"
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't there also capital?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but it isn't used in the static data and before we didn't have an enum for it - the only place currently that could use it is achievements which we can update to use + clanCapital

coc/enums.py Outdated
"Most Valuable Clanmate",
]

UNRANKED_LEAGUE_DATA = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this go into the constants?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically yes, but check generate_constants in the static updater and let me know what you think then

Copy link
Collaborator

@doluk doluk left a comment

Choose a reason for hiding this comment

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

looking good to me

@doluk doluk merged commit ec07b5a into mathsman5133:master Dec 6, 2025
2 of 7 checks passed
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.

2 participants