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

@webknjaz webknjaz commented Nov 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Ref #277

@webknjaz webknjaz requested a review from Mariatta as a code owner November 20, 2018 17:13
@webknjaz webknjaz mentioned this pull request Nov 20, 2018
Closed
@webknjaz webknjaz force-pushed the bugfix/consistent-config-cherry-picker branch from 8b05f5b to df45585 Compare November 23, 2018 15:12
@webknjaz

webknjaz commented Nov 23, 2018

Copy link
Copy Markdown
Member Author

@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
Member Author

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
Member Author

Cool, thanks :)

@webknjaz

webknjaz commented Dec 7, 2018

Copy link
Copy Markdown
Member Author

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

@webknjaz

webknjaz commented Dec 17, 2018

Copy link
Copy Markdown
Member Author

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
Member Author

@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
Member Author

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

@asvetlov asvetlov 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 thread cherry_picker/cherry_picker/cherry_picker.py Outdated
Comment thread cherry_picker/cherry_picker/cherry_picker.py Outdated
@webknjaz

webknjaz commented Jan 3, 2019

Copy link
Copy Markdown
Member Author

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
Member Author

Great, thanks @Mariatta!

@webknjaz webknjaz force-pushed the bugfix/consistent-config-cherry-picker branch 3 times, most recently from f4f8d67 to a207fb9 Compare January 7, 2019 18:37
@webknjaz webknjaz force-pushed the bugfix/consistent-config-cherry-picker branch from 4b63d05 to dda278d Compare February 9, 2019 23:39
@webknjaz webknjaz force-pushed the bugfix/consistent-config-cherry-picker branch from dda278d to 1a5d76f Compare February 9, 2019 23:52
@webknjaz

webknjaz commented Feb 10, 2019

Copy link
Copy Markdown
Member Author

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

@webknjaz webknjaz changed the title [WIP] [PoC] Initial implementation of storing state in Git config Implement storing runtime state in repo level Git config Feb 10, 2019
@webknjaz

webknjaz commented Feb 10, 2019

Copy link
Copy Markdown
Member Author

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

Mariatta
Mariatta reviewed Feb 12, 2019
View reviewed changes
Comment thread cherry_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
Member Author

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

@asvetlov asvetlov 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
Member Author

Sure, on it

@webknjaz

webknjaz commented Feb 21, 2019

Copy link
Copy Markdown
Member Author

done!

@Mariatta Mariatta merged commit b4773c9 into python:master Feb 21, 2019
@Mariatta

Mariatta commented Feb 21, 2019

Copy link
Copy Markdown
Member

Thanks! Will release soon!

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

Reviewers

@Mariatta Mariatta Mariatta left review comments

@asvetlov asvetlov asvetlov 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