added a bullet dict class as #27 requested#30
added a bullet dict class as #27 requested#30CurtisPreston99 wants to merge 6 commits intobchao1:masterfrom
Conversation
the way this works is you define a dict the keys are shown to the user but the values are returned when selected. i think this would be nice to have added it to client file as class BulletDict, and set up in init, also added an example feel free to give me advice on how to improve this class
bullet/__init__.py
Outdated
| from .client import SlidePrompt | ||
| from .client import ScrollBar No newline at end of file | ||
| from .client import ScrollBar | ||
| from .client import BulletDic No newline at end of file |
There was a problem hiding this comment.
Your class is named BulletDict, not BulletDic
examples/BulletDic.py
Outdated
| @@ -0,0 +1,16 @@ | |||
| from bullet import BulletDic | |||
bullet/client.py
Outdated
|
|
||
|
|
||
| @keyhandler.init | ||
| class BulletDict: |
There was a problem hiding this comment.
This is a lot of repeated code for what seems like it should just be a child class that overrides accept().
There was a problem hiding this comment.
ah okay that makes sense ill do that now thank you.
There was a problem hiding this comment.
what are your thoughts on it now? any improvements you would recommend me trying to do?
fixing what was suggested by rcfox on pull request bchao1#27
|
(I should point out that I have no authority on this project, I'm just a random guy commenting on someone else's repo.) After sleeping on this, it seems like it would be best if this were rolled into the main classes. (Sorry for the runaround) It's simple enough to detect the type of variable being passed into the constructor. It also seems like list choices would be the special case. If you get a list, you could just make a dict out of it: |
|
Actually, it would probably be better to store choices as a list of (key, value) pairs. That way duplicate list items won't be lost, and you avoid the weirdness of trying to index into a dictionary at a certain position. |
|
thats okay im just trying to improve my code, it was my new years resolution to contribute to things, also im still at University so learning better ways of doing things is always nice. also that two list idea seams pretty good so you would just have a list for displaying and a list for return values |
rcfox
left a comment
rcfox
left a comment
There was a problem hiding this comment.
Mostly looks good, just needs a bit of polish.
|
|
||
| #test flag also return objects | ||
| self.objects=[] | ||
| if(type(choices) is dict): |
There was a problem hiding this comment.
isinstance(obj, cls) is generally preferable to type(obj) is cls since it supports subtypes. (For instance, OrderedDict)
| self.objects=[choices[x] for x in self.choices] | ||
| else: | ||
| self.choices = choices | ||
|
|
There was a problem hiding this comment.
If you set self.objects = choices, you wouldn't need the extra check in accept(), you could also just return from self.objects
bullet/client.py
Outdated
| self.objects=[] | ||
| if(type(choices) is dict): | ||
| self.choices = list(choices.keys()) | ||
| self.objects=[choices[x] for x in self.choices] |
There was a problem hiding this comment.
How about list(choices.values()) for consistency?
There was a problem hiding this comment.
did not know that there was a .values() function
bullet/client.py
Outdated
| cursor.show_cursor() | ||
| ret = self.choices[self.pos] | ||
| #flag check and return ether objects or just choices | ||
| if(self.objects): |
There was a problem hiding this comment.
Nitpicking, but you generally leave off parentheses on if statements in Python for simple conditions like this.
Also, try to be consistent with whitespace to match the surrounding code.
removed flag statement from accept by making objects the same as choices if its a list
3 participants
the way this works is you define a dict the keys are shown to the user but the values are returned when selected. i think this would be nice to have.
added it to client file as class BulletDict, and set up in init, also added an example feel free to give me advice on how to improve this class