search

scottmconway / shopgoodwill-scripts

A collection of scripts for programmatically interacting with Shopgoodwill
GNU General Public License v3.0
51 stars 17 forks source link

Interested in PR? #25

Open rsnodgrass opened 9 months ago

rsnodgrass commented 9 months ago

Feel free to ignore!

scottmconway commented 9 months ago

Thanks for the contribution! I'm interested in some of your changes, but multiple PRs may be preferred here for code chsnges vs formatting changes.

I currently format my code with black, which uses double quotes for strings by default. I don't want to merge changes that use a different set of formatting rules.

I'll make additional comments on this in a few days, but to get it in a more mergable state, please run black and isort on all Python files in the fork.

I saw that the logging import moved to the top of the list of imports in one or more files - I'm not sure why that would occur.

rsnodgrass commented 9 months ago

I’ll rerun this with black (instead of brunette which is a black fork with configurable single or double quotes).

On Feb 16, 2024 at 6:59 PM, <Scott Conway @.***)> wrote:

Thanks for the contribution! I'm interested in some of your changes, but multiple PRs may be preferred here for code chsnges vs formatting changes.

I currently format my code with black, which uses double quotes for strings by default. I don't want to merge changes that use a different set of formatting rules.

I'll make additional comments on this in a few days, but to get it in a more mergable state, please run black and isort on all Python files in the fork.

I saw that the logging import moved to the top of the list of imports in one or more files - I'm not sure why that would occur.

— Reply to this email directly, view it on GitHub (https://github.com/scottmconway/shopgoodwill-scripts/pull/25#issuecomment-1949619284), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAQY4XGJC4GCD5DJJM66WR3YUAMINAVCNFSM6AAAAABDM6JSYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBZGYYTSMRYGQ). You are receiving this because you authored the thread.Message ID: @.***>

rsnodgrass commented 9 months ago

Ok, all formatting changes from brunette have been reverted and just the manual changes are in the PR now. Plus the additional files (docs, simpler example without logging, etc).

scottmconway commented 8 months ago

Ok, I have more thoughts on this:

It looks like a decent amount of this PR refactors variable names and changes comments, and another part adds features (makes it a proper module, adds environment variable support, etc).

Again, I think it should be split into multiple PRs. I'm not really keen to accept refactor changes right off the bat, but features are certainly acceptable.