stripe / pg-schema-diff

Go library for diffing Postgres schemas and generating SQL migrations
MIT License
270 stars 20 forks source link

fix: #67 converting all tabs to spaces in migration acceptance tests #139

Closed ammiranda closed 3 days ago

ammiranda commented 2 weeks ago

Description

Standardizing the whitespace in the migration_acceptance_tests to be spaces so viewing isn't uneven.

Motivation

Resolves #67

Testing

I ran the test docker image built on my local machine and observed no test failures which is expected given the only changes are whitespace. The one caveat is it was only run against postgres 14 given that was the specified arg in the test dockerfile.

CLAassistant commented 2 weeks ago

CLA assistant check
All committers have signed the CLA.

ammiranda commented 1 week ago

@bplunkett-stripe ok I think the latest push is getting closer to what is desired. One thing that was interesting is if I converted the tabs to spaces for the opening ` in the first few files the lint would give goimport-ed failures, if they are left alone the lint passes. Any guidance on what to do for this is appreciated because it feels a bit wonky, I could adjust it so the tab replacement doesn't happen for the lines that solely have the ` characters but don't know if that is acceptable. Also I achieved this through a python script that I can add into the source so it can be run in the future easily (possibly in its own docker container) so just let me know if I should push it up.

bplunkett-stripe commented 1 week ago

ok I think the latest push is getting closer to what is desired. One thing that was interesting is if I converted the tabs to spaces for the opening in the first few files the lint would give goimport-ed failures, if they are left alone the lint passes. Any guidance on what to do for this is appreciated because it feels a bit wonky, I could adjust it so the tab replacement doesn't happen for the lines that solely have the characters but don't know if that is acceptable. Also I achieved this through a python script that I can add into the source so it can be run in the future easily (possibly in its own docker container) so just let me know if I should push it up.

Hmmm I see....Instead of converting tabs to spaces, what if we converted spaces to tabs? That would also align with go formatting recommendation and give us consistent white space throughout. I'd be interested in seeing the script!

ammiranda commented 1 week ago

ok I think the latest push is getting closer to what is desired. One thing that was interesting is if I converted the tabs to spaces for the opening in the first few files the lint would give goimport-ed failures, if they are left alone the lint passes. Any guidance on what to do for this is appreciated because it feels a bit wonky, I could adjust it so the tab replacement doesn't happen for the lines that solely have the characters but don't know if that is acceptable. Also I achieved this through a python script that I can add into the source so it can be run in the future easily (possibly in its own docker container) so just let me know if I should push it up.

Hmmm I see....Instead of converting tabs to spaces, what if we converted spaces to tabs? That would also align with go formatting recommendation and give us consistent white space throughout. I'd be interested in seeing the script!

@bplunkett-stripe I just pushed up the change changing 4 spaces to tabs. You can view the python script as a gist here: https://gist.github.com/ammiranda/44c720878f09aad21e8e17e305988537. Let me know if you want me to write a Dockerfile to enable it to be easily run as part of this PR!

bplunkett-stripe commented 5 days ago

@bplunkett-stripe I just pushed up the change changing 4 spaces to tabs. You can view the python script as a gist here: https://gist.github.com/ammiranda/44c720878f09aad21e8e17e305988537. Let me know if you want me to write a Dockerfile to enable it to be easily run as part of this PR!

Nice changes, look solid! Thinking about it a bit more...I think we want to convert the tabs to spaces, i.e., reverse your script! Sorry for the confusion on that! Since you wrote a script, hopefully a pretty easy thing to switch up! Should be good to merge after that.

Thanks for the script! might look into vendoring your script in later. I'm a little hesitant since the matching condition is pretty loose.

-- Sorry again for the churn! I'm hoping this won't be too annoying with the script you wrote.

ammiranda commented 5 days ago

@bplunkett-stripe I've switched it back to spaces, please review again when able. If you can let me know how you'd want me to tighten the script I can work on implementing it, it seems to work for the existing formatting FWIW but I understand the desire to not be sloppy.

ammiranda commented 3 days ago

Got it, if you create an issue with your specifications I can work on that separately in a new PR. Or if you want me to create it I can do that.

bplunkett-stripe commented 3 days ago

Doing as a separate PR is probably best. I think what we're aiming for here is to just have a golang "script" that: