Skip to content

Conversation

@webknjaz
Copy link
Member

@webknjazwebknjaz commented Nov 20, 2018

Ref #277

@webknjaz
Copy link
MemberAuthor

@Mariatta@abadger I've cleaned things up a bit. Maybe it's time to move helper functions into util module/package or is it a task for separate PR?

@webknjaz
Copy link
MemberAuthor

webknjaz commented Nov 27, 2018

Hi @Mariatta,

I'm currently waiting for your approval wrt architectural changes. After that I'll proceed with adjusting/adding tests.

P.S. @abadger suggested that I could add a CLI command for git config --local --remove-section cherry-picker but I'm doubtful: it's a dangerous thing for the flow and providing users with such UI might encourage them to overuse it where they don't understand consequences. Ideally, that failure shouldn't be happening during the normal process at all.

@Mariatta
Copy link
Member

Sorry for the delay. Will take a look in the weekend.

@webknjaz
Copy link
MemberAuthor

Cool, thanks :)

@webknjaz
Copy link
MemberAuthor

Hey @Mariatta, any 🤔💭 on this so far?

@webknjaz
Copy link
MemberAuthor

Hi @Mariatta, any comments?

@Mariatta
Copy link
Member

So sorry, I think I'm not able to effectively review this PR.
Is there anyone else who might be able to help review this?

@webknjaz
Copy link
MemberAuthor

@Mariatta
Well, @abadger said that it's fine. We just wanted to check that it's fine with you since you're the maintainer.

@webknjaz
Copy link
MemberAuthor

I'll ask @asvetlov to take a look as well, then, to have more eyes on it.

Copy link
Contributor

@asvetlovasvetlov left a comment

Choose a reason for hiding this comment

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

The idea looks interesting.
Would you add tests for it?
At least we have several ones in test.py already.

@webknjaz
Copy link
MemberAuthor

Thanks for the review, Andrew! I just wanted to wait for architectural approval before investing some time into tests. Now I can proceed :)

@Mariatta
Copy link
Member

Thanks @webknjaz and @asvetlov. For me, as long as there is no change to how it works for CPython /miss-islington’s purpose, then I'm good with it.

@webknjaz
Copy link
MemberAuthor

Great, thanks @Mariatta!

@webknjazwebknjazforce-pushed the bugfix/consistent-config-cherry-picker branch 3 times, most recently from f4f8d67 to a207fb9CompareJanuary 7, 2019 18:37
@webknjazwebknjazforce-pushed the bugfix/consistent-config-cherry-picker branch from 4b63d05 to dda278dCompareFebruary 9, 2019 23:39
@webknjazwebknjazforce-pushed the bugfix/consistent-config-cherry-picker branch from dda278d to 1a5d76fCompareFebruary 9, 2019 23:52
@webknjaz
Copy link
MemberAuthor

Update: with the recent commits I've hit 83% test coverage.

@webknjazwebknjaz changed the title [WIP] [PoC] Initial implementation of storing state in Git configImplement storing runtime state in repo level Git configFeb 10, 2019
@webknjaz
Copy link
MemberAuthor

@Mariatta@abadger@asvetlov I believe this is ready for the final review :)

Co-Authored-By: webknjaz <wk.cvs.github@sydorenko.org.ua>
@Mariatta
Copy link
Member

It seems ok to me. Would @abadger or @asvetlov be able to do more thorough review?

@webknjaz
Copy link
MemberAuthor

Thanks. I'll ping them. I think Andrew is away for a couple of days, though...

Copy link
Contributor

@asvetlovasvetlov left a comment

Choose a reason for hiding this comment

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

LGTM now

@Mariatta
Copy link
Member

Thanks @asvetlov and @webknjaz. If you could update the release notes and up the version, I can merge and get it deployed to PyPI.

@webknjaz
Copy link
MemberAuthor

Sure, on it

@webknjaz
Copy link
MemberAuthor

done!

@MariattaMariatta merged commit b4773c9 into python:masterFeb 21, 2019
@Mariatta
Copy link
Member

Thanks! Will release soon!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@webknjaz@Mariatta@asvetlov@the-knights-who-say-ni