Using Player class, added enable/disable buttons #26
Conversation
This commit changes some aspects of the application: - created Player class to hold information regarding a player - Not highlighting the lines when an UPDATE or SELLING events is triggered (avoid the view to be scrolled up and down) - The bid view does not save anymore players - Saving mechanism has changed: now using pickle. This means that this will break backwards compatibility with previous version of the players.json - Added enabled/disabled buttons for each player in the playersearch.py - Code cleanup
| self.add_player(item, write=False) | ||
|
|
||
| class Player(): |
There was a problem hiding this comment.
I would break this out into it's own file.
|
Overall, I'm fine with the notion of breaking more of our dicts out into their own objects. We should come up with a file structure for classes that can then be imported into the UI vs declaring them inline, however. I think backwards compatibility is a HUGE thing. v0.7.1-beta has had over 100 downloads, and v0.7.0-beta had over 300 before I removed the download links (see release api)! If it were just a couple people using it, then I wouldn't mind a backwards compatibility break, but it's more than that. JSON is also much easier to read than a pickled object. You can open up the file yourself and make some changes. I propose sticking with json and adding serialization methods to the Player class in order to convert it to JSON for storage. The way your player class is defined only has the attributes we currently are using as well. I chose to store the entire player search response from EA just in case we wanted to use some of the other attributes later on down the road. Maybe we have a metaData dictionary on the player object so those attributes are still accessible? Lastly, I would love to see support for loading and saving different player lists in the UI. This would enable people to save a standard version of their lists and remove/disable players for a single session (or maybe export the new list) without modifying the original copy. My idea would be to keep the current active configuration for bidding in |
|
I will work on the serialisation, then. At the beginning I did that and I found very problematic the de-serialisation. If you work with dictionaries, then you are always "hoping" that the key exists. Using classes this is somehow ensured by the concept of class. If you look carefully at my class, there is a I agree that the JSON file is easier to read but that's out of the scope of the tool. It is actually quite hard, at the moment, to manually modify the players.json file. Anyway, I will try to polish the current codebase and try to see what I can do with the (de)serialisation. |
|
Thanks for the updates. I'm going to test this out tonight before merging into master since it's a fairly large change. |
|
I am not sure this can be merged as it is. Please report back any problem.. I want to be super-sure before merging to master :) |
|
Alright, I'll be sure to test throughly ;) |
|
@ElBryan: I'm swamped at work right now, so I probably won't have a chance to review until around Christmas. |
This commit changes some aspects of the application:
@hunterjm I know that your first instinct here would be to not merge this PR. Therefore, I am kindly asking you to give me feedback on the code so that we can work this out together. The idea here is that we are using a class, therefore we can push this object across the views and avoid accessing fields by string (a-la dictionary).Let me know what are your feelings on this commit.