Talent point ranking

[dazer]

This is the best I could come up with. I'm not using self.talents.talent_name because the approach I took was to set new values passing the talent name string. I'm open to suggestions to change it since those try:/except: look a bit nasty (or naïve if I may say).

The 31/41 logic is no problem since those values are checked only when building the talents objects: they all can be set to 0 at any point during calculations.

The sorting is done just like in the test files but happens inside the method if you pass the print parameter. I don't think a dictionary is sortable (or even if the concept of sorting makes sense for them) , so that's the first thing that crossed my mind.

It works and throws a reasonable output albeit slowly: it runs the dps engine for talents that are expected to return 0.0 (this was the reason for me asking whether passing the talents beforehand would be a smart idea). Only fail I don't see an easy workaround for is RvS: it says it's compulsory, when it actually is not, but that's only because, well, it is mandatory if you set the cycle to use it.

[julienp]

Couple things that strike me as "unpythonic": Don't change the signature of __getattr__. You could have a method get_talent_tier(talent_name) on ClassTalents. Also, don't use it like object.__getattr__(name), instead use getattr(object, name).

I'd make get_ranking_for_talents always return talents_ranking, not_implemented_talents and put the printing functionality in an another function, having it sometimes return something and sometimes print depending on an argument seems weird. Maybe rename it get_talents_ranking.

Regarding the try/except to find the right tree, ClassTalents has a dictionary treeForTalent that has the talents as keys and the right talent tree as values, so something like self.talents.treeForTalent[talent_name].set_talent(talent, new_talent_value) should do the trick (not tested), it's not a really prettier ... maybe ClassTalents should have a __setattr__ that does the right thing, just like its __getattr__?

[dazer]

Fixed some of that. I'll look into the talent tier and __getattr__ __setattr__thingies later. As for the printing function, where should that be located?

[dazer]

I think it now looks about right. Setting new_talent_value through that long call doesn't seem as intimidating as my previous iteration. If someone is interested in going back to the talent objects and define __setattr__ it could look a little better, but I doubt it will be needed anywhere else, so I find this to be a 'good enough' approach.

I did some further thinking on the 31/41 logic: if this works without further ado that's because it gets checked on build objects time as to say and it doesn't update when changing it in calculation time. I imagine someone could go and rewrite the talents and make it check this every time they are accessed (and render get_talents_ranking useless), but I think that'd be overkilling it. Anyway, if you find any value in forcing this to work under rewritten talent objects I believe the best approach would be setting a new dummie_talent with value 0 (and maybe tier 8, or 0) for each tree in build time and set it to 1 or -1 (in the right tree) when entering every new_talent_value; would that be usefull?

[Aldriana]

Couple of quick thoughts on the 31/41 issue:

We could get around the 31 issue by just detecting which tree we're using by which one has the most points rather than always checking for 31 points. This should be just as good in every circumstance where it matters.

For 41... I guess, upon contemplation, its not clear to me that we should be enforcing "correctness" of talent trees to start with. Would it make sense to leave that check out entirely?

I guess the only problem is that if someone fills in something stupid like 20/20/20 something bizarre will happen as a result, but I'm not sure how much effort its worth putting into supporting obviously invalid input.

[dazer]

In fact the 31points was never checked: it already took the tree with the most talents in it. I deprecated the 41 check in my last commit: I agree this doesn't need to be checked by our end but rather (if ever) by the front end; similarly, if some other checks should be done (like cold blood being a prerequisite to seal fate or 5talent points per tier) it belongs in the front end.

The 'You must have 31 points in at least one talent tree.' exception is never reached if the talent strings in the input are a little less than stupid.

Sorting the ranking (definig a print function that is) aside, I think this is about ready. Should I build such a print function or let it live in the test files as is?

edit: I have seen a test for ep values in the test suite so I'm gessing this one could use one, but I'm far from understanding how those work.

[Aldriana]

Some users will want to sort by value, and some will want to sort by talent tier, so I'm inclined to leave it unsorted and let the caller handle it.