Skip to content
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

collect_prs_info: include full data #41

Merged
merged 3 commits into from
Jan 13, 2020
Merged

Conversation

simahawk
Copy link
Contributor

@simahawk simahawk commented Dec 4, 2019

I don't see any good reason not to add full info here.

@coveralls
Copy link

coveralls commented Dec 4, 2019

Coverage Status

Coverage increased (+0.2%) to 81.572% when pulling 56cb704 on simahawk:patch-1 into dfdb3db on acsone:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 81.386% when pulling 1d9355b on simahawk:patch-1 into dfdb3db on acsone:master.

@sbidoul
Copy link
Member

sbidoul commented Dec 4, 2019

I don't immediately visualize the impact of the change.
It would help if you could provide an example output before and after the PR.

@simahawk
Copy link
Contributor Author

simahawk commented Dec 5, 2019

Sorry, had no time to explain 😅
Typical use case: you want to show the title or some other info that right now is excluded (as I want to do).
At first I thought: ok, let's add the title and allow to pass extra key names to the method.
But then: why? What's the issue w/ exposing detailed information from a PR given that this won't change anything in how this tool works?

Regarding the output: is quite big, it doesn't fit here :)

Some more details on how we use it: in our project cookiecutter template we have some invoke tasks that wrap submodules and pending merges. One of them is show-prs that prints all pending merge PRs w/ meaningful information.

@simahawk
Copy link
Contributor Author

simahawk commented Jan 8, 2020

@sbidoul do you visualize it now? 😄

Copy link

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@sbidoul
Copy link
Member

sbidoul commented Jan 13, 2020

@sbidoul do you visualize it now?

Not really actually. I checked again and I don't see the CLI exploiting this.

Are you using git-aggregator as a library?

@simahawk
Copy link
Contributor Author

@sbidoul yes, sorry, I should have mentioned this to give you a better ctx.
This change helps displaying more info.
If you prefer I can add an optional argument to make this behavior optional, but I don't see really the issue of including more info in std behavior as long as it does not change anything else.

@sbidoul
Copy link
Member

sbidoul commented Jan 13, 2020

Ok, I'll merge this... but beware that this project has never been meant to be used as a library: there is no stable API, no doc and no tests for that. So things may change and break without notice.

@sbidoul
Copy link
Member

sbidoul commented Jan 13, 2020

Ok, I'll merge this

Hm, no looking closer, there is a risk that other pr_info keys that were set before are overwritten by the update. Perhaps you could you store the raw GitHub data in a separate key?

@simahawk
Copy link
Contributor Author

@sbidoul better?

@sbidoul sbidoul merged commit 06ebfe6 into acsone:master Jan 13, 2020
@sbidoul
Copy link
Member

sbidoul commented Jan 13, 2020

Yes, thanks!

@simahawk
Copy link
Contributor Author

simahawk commented Feb 3, 2020

@sbidoul could you release on pypi? 🙏 🤗

@sbidoul
Copy link
Member

sbidoul commented Feb 3, 2020

ok, 1.7.2 is on it's way

@simahawk
Copy link
Contributor Author

simahawk commented Feb 3, 2020

merci beacoup 🤝

@simahawk
Copy link
Contributor Author

@sbidoul 1.7.2 is lost on its way 🤣

@sbidoul
Copy link
Member

sbidoul commented Feb 25, 2020

@simahawk
Copy link
Contributor Author

@sbidoul changelog + commit history are not saying anything about it 🤷‍♂️

image

@sbidoul
Copy link
Member

sbidoul commented Feb 25, 2020

Ah right, sorry about that. I was not sure what to put in the changelog because it's an internal change to a undocumented unsupported internal API that people should not rely on :) But I should have added a changelog entry nevertheless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants