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.

Sign up for GitHub

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

Jump to bottom

Implement storing runtime state in repo level Git config #295

Merged
merged 36 commits into from
Feb 21, 2019
Merged

Implement storing runtime state in repo level Git config #295

merged 36 commits into from
Feb 21, 2019

Conversation

webknjaz
Copy link
Contributor

@webknjaz webknjaz commented Nov 20, 2018 •
edited
Loading

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
Copy link
Contributor 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
Copy link
Contributor Author

webknjaz commented Nov 27, 2018 •
edited
Loading

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

Cool, thanks :)

@webknjaz
Copy link
Contributor Author

webknjaz commented Dec 7, 2018

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

@webknjaz
Copy link
Contributor Author

Hi @Mariatta, any comments?

@Mariatta
Copy link
Member

Mariatta commented Jan 3, 2019

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

webknjaz commented Jan 3, 2019

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

webknjaz commented Jan 3, 2019

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

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

cherry_picker/cherry_picker/cherry_picker.py Outdated Show resolved Hide resolved
cherry_picker/cherry_picker/cherry_picker.py Outdated Show resolved Hide resolved
@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 3, 2019

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

Mariatta commented Jan 3, 2019

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

webknjaz commented Jan 3, 2019

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
Copy link
Contributor 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
Copy link
Contributor Author

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

@Mariatta
Copy link
Member

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

@webknjaz
Copy link
Contributor 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
Copy link
Contributor

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

Sure, on it

@webknjaz
Copy link
Contributor Author

done!

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

Thanks! Will release soon!

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