search

adrienverge / localstripe

A fake but stateful Stripe server that you can run locally, for testing purposes.
GNU General Public License v3.0
192 stars 59 forks source link

Basic support for Price #194

Closed t3hmrman closed 3 years ago

t3hmrman commented 3 years ago

Unfortunately a bunch of changes that aren't strictly related to the title in here from my fork (most of them more descriptive errors and stuff) -- but support for Prices is in here.

Please let me know which changes you don't like (if you like the error messages as they are, etc) and I'll try to cut down the PR -- as of now this code works for my uses (I run through adding customers, products, prices, creating a subscription, activation and deactivation, payment methods in my testing). Official tests pass as well so at least nothing that was being tested is broken, though I can't say I thought too much about the logic

t3hmrman commented 3 years ago

Well evidently autopep8 wasn't enough to get all the lint issues some lines are still over 79 characters (nevermind all the semantic changes it made like moving type() to isinstance) -- is there a known-trusted formatter that I can run? black/yapf?

I ran yapf and it changed the code even more than autopep8 did.

I'm going to back out the autopep8 change for now. since it makes the PR way noisier.

H--o-l commented 3 years ago

Hi @t3hmrman!

Thanks for opening a PR for localstripe!

However, I have a few things to ask you.

First, can you avoid opening unfinished PR or pushing on branches of opened PR? It spams a lot. Try to open PRs only when you are 99% sure lint/format/commit etc are ready.

Besides this remark, for us to be able to review your PR and hopefully accept it, you should make them as easy as possible to review (it should take us the least time possible). So, as you proposed, you should cut down the PR to the essential. Also, still in the idea to help us understand your PR and review it, each commit of the PR should add or change one feature (or one resource, or one parameter) and only one. You can do multiple small and simple PRs if you think it will help us to understand and review them. It also means that commits like Run autopep8 then Revert "Run autopep8" should not appear in the tree.

About cosmetic change or improvement of error messages: If you really want to propose them, please open a dedicated PR for that, where we will be able to talk and evaluate the idea.

Finally, put as much explanation or detail as possible in your commits messages. The commit is the key piece in a git repository to track change long term. Each commit should be as complete and as simple as possible.

is there a known-trusted formatter that I can run?

No, you need to try to follow the existing style. You can run flake8 like the CI does to check if it's OK. If you open a PR and the style is 90% good, we will help you for the last 10%.

t3hmrman commented 3 years ago

Hey @H--o-l thanks for the explanation and putting together that detailed response -- I'll close this PR and try to find time to cut it down and try to make and submit two separate PRs (this time with the error message changes and other stuff separated out)!

H--o-l commented 3 years ago

:ok_hand: sounds good, thanks @t3hmrman!