- Notifications
You must be signed in to change notification settings - Fork673
Fix for issue #717#727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…fallback for missing Event parameters.
| @@ -357,7 +357,7 @@ def display_dict(d, padding): | |||
| id = getattr(obj, obj._id_attr) | |||
| print('%s: %s' % (obj._id_attr.replace('_', '-'), id)) | |||
| if hasattr(obj, '_short_print_attr'): | |||
| value = getattr(obj, obj._short_print_attr) | |||
| value = getattr(obj, obj._short_print_attr) or 'None' | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn'tNone events just be skipped in the CLI?
target_title: Nonetarget_title: Nonetarget_title: Nonetarget_title: Nonetarget_title: Nonetarget_title: Nonetarget_title: Nonetarget_title: Nonetarget_title: NoneThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, I don't know what these bad events are. It seems to me better to display them then to hide them. Because I consider github as a tool that helps me to show precisely what's going on in GitLab. If it hides the bad results, it may hide an important clue to a problem I'm solving.
But I don't know how other parts of github behave. It's up to you to decide what you want it to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The problem I see with this issue is that there is no way to have a usable output without displaying multiple attributes. For events that would be target_type and action_name for exemple
What we could do (to be discussed):
- make the
display_dictcheck for areprmethod for the object. This method would return a key and a value to be displayed. We could use__repr__ou__unicode__for this maybe - if the method doesn't exist, fallback to the
_short_print_attrattribute - if this attribute is None raise an error so we can fix the code and add the method from 1 (not sure about this step, it's not nice for end users).
Other idea:_short_print_attr could become a tuple, and we display multiple lines when needed
@xarx00 could you make the commit messages a bit more meaningful? Something like: Thanks for your help on fixing the CLI 👍 |
xarx00 commentedMar 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@gpocentek OK, I have thought that link to the defect is enough. Also, the PEP8 test on GitHub Travis CI complains when the commit message is longer than 50 chars. |
I propose this fix for isuue#717. It fixes
event list, which didn't worked when invoked from CLI (and probably neither when used from gitlab API).