-
Notifications
You must be signed in to change notification settings - Fork 3
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New functionality from all forks #1
base: master
Are you sure you want to change the base?
Conversation
…'s name filter doesn't work
@@ -187,7 +271,7 @@ def parse_by_type(node): | |||
return value | |||
|
|||
elif obj_type == "commalist": | |||
value = node.childNodes[0].wholeText.strip() | |||
value = ''#node.childNodes[0].wholeText.strip() |
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.
What's the reason for using empty string here?
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.
Before I merge this in, I'd like to confirm the reasoning behind this change - was this some stray debugging that accidentally got committed?
hey Ian, thanks for the rollup here! I did a quick scan through and this looks good except for a question I had about an empty string and dead comment. I'm happy to have you maintain this package, as I haven't used Pivotal in a long time myself. What is your pypi username? You can PM it to me if you prefer. |
I haven't brought the name failure with the pivotal team as the version of the API the package targets is now deprecated. The current version (v5) is JSON rather than XML, so this package will need a re-write. If the bug is still present after that process, I shall certainly report it. As far as the string is concerned, that is from a commit that one of the people I am rolling up has made, so I've no idea on the motivation there. I'll get in touch the committer and find that out at some point. Or possibly just test without and remove it. |
Hey Ian, sorry this fell off my radar. I can add you as a publisher for the pypi module, but what do you want to do about Github? I could either point folks to your fork, or make you a collaborator here. |
Content preview: I suppose that depends upon whether you are likely to do anything Content analysis details: (-2.9 points, 5.0 required) pts rule name description -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP I suppose that depends upon whether you are likely to do anything Regards,Ian Gibbs On 01/12/14 05:16, Matt Pizzimenti wrote:
|
To keep things simple I'll just add you as a collaborator here for now (since all the other forks are off this one anyway). I had one last question about that empty string before merging. I added you as a collaborator in the meantime :) |
Hmm, I'm not sure that comment ended up directed at the original author as I intended. I'll try again. |
@realflash did you ever figure that last bit out? I'll give the preemptive assuming you can confirm things with that author. Once you confirm with that author, you should have access to merge this PR and publish :) happy new year |
Hi Matt, The author did get back to me; not unreasonably given the commit was I had intended to test it but I haven't got around to it yet. Hopefully |
Dear mjpizz,
Various people have forked this module and built on it, including me. This pull request is all subsequent work. As of the latest commit, it is now tested against v4 of the API and has additional functionality for activity, tasks and moving stories. Please could you merge and update the pypi entry; as of now 0.0.3 doesn't work because it points at the v2 API, which Pivotal has retired.
If you would like someone to adopt the package I'd be happy to do so.