Skip to content

Implement storing runtime state in repo level Git config - #295

Merged
Mariatta merged 36 commits into
python:masterfrom
webknjaz:bugfix/consistent-config-cherry-picker
Feb 21, 2019
Merged

Implement storing runtime state in repo level Git config#295
Mariatta merged 36 commits into
python:masterfrom
webknjaz:bugfix/consistent-config-cherry-picker

Conversation

@webknjaz

@webknjazwebknjaz commented Nov 20, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Ref #277

@webknjazwebknjaz mentioned this pull request Nov 20, 2018
Closed
@webknjaz
webknjazforce-pushed the bugfix/consistent-config-cherry-picker branch from 8b05f5b to df45585CompareNovember 23, 2018 15:12
@webknjaz

webknjaz commented Nov 23, 2018

Copy link
Copy Markdown
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

webknjaz commented Nov 27, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

Mariatta commented Nov 30, 2018

Copy link
Copy Markdown
Member

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

@webknjaz

webknjaz commented Nov 30, 2018

Copy link
Copy Markdown
MemberAuthor

Cool, thanks :)

@webknjaz

webknjaz commented Dec 7, 2018

Copy link
Copy Markdown
MemberAuthor

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

@webknjaz

webknjaz commented Dec 17, 2018

Copy link
Copy Markdown
MemberAuthor

Hi @Mariatta, any comments?

@Mariatta

Mariatta commented Jan 3, 2019

Copy link
Copy Markdown
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

webknjaz commented Jan 3, 2019

Copy link
Copy Markdown
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

webknjaz commented Jan 3, 2019

Copy link
Copy Markdown
MemberAuthor

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

asvetlov
asvetlov reviewed Jan 3, 2019
View reviewed changes

@asvetlovasvetlov left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadcherry_picker/cherry_picker/cherry_picker.py Outdated
Comment threadcherry_picker/cherry_picker/cherry_picker.py Outdated
@webknjaz

webknjaz commented Jan 3, 2019

Copy link
Copy Markdown
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

Mariatta commented Jan 3, 2019

Copy link
Copy Markdown
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

webknjaz commented Jan 3, 2019

Copy link
Copy Markdown
MemberAuthor

Great, thanks @Mariatta!

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

webknjaz commented Feb 10, 2019

Copy link
Copy Markdown
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

webknjaz commented Feb 10, 2019

Copy link
Copy Markdown
MemberAuthor

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

Mariatta
Mariatta reviewed Feb 12, 2019
View reviewed changes
Comment threadcherry_picker/cherry_picker/test.py Outdated
@Mariatta@webknjaz
f-stringify concatenation in tests
af69d6d
Co-Authored-By: webknjaz <wk.cvs.github@sydorenko.org.ua>
@Mariatta

Mariatta commented Feb 13, 2019

Copy link
Copy Markdown
Member

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

@webknjaz

webknjaz commented Feb 13, 2019

Copy link
Copy Markdown
MemberAuthor

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

asvetlov
asvetlov approved these changes Feb 21, 2019
View reviewed changes

@asvetlovasvetlov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM now

@Mariatta

Mariatta commented Feb 21, 2019

Copy link
Copy Markdown
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

webknjaz commented Feb 21, 2019

Copy link
Copy Markdown
MemberAuthor

Sure, on it

@webknjaz

webknjaz commented Feb 21, 2019

Copy link
Copy Markdown
MemberAuthor

done!

@Mariatta
Mariatta merged commit b4773c9 into python:masterFeb 21, 2019
@Mariatta

Mariatta commented Feb 21, 2019

Copy link
Copy Markdown
Member

Thanks! Will release soon!

@webknjazwebknjaz mentioned this pull request Feb 10, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MariattaMariattaMariatta left review comments
@asvetlovasvetlovasvetlov approved these changes

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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