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

[ammiranda]

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]

All committers have signed the CLA.

[bplunkett-stripe]

The change to the contributing.md I believe is not correct.

Also, only the SQL strings should be changed. The actual code should not have substitutions made. The goal here is to consistent spacing on the SQL strings, since the linter does not enforce anything in them.

[bplunkett-stripe]

@ammiranda, the code is still formatted with tabs instead of spaces. Please do not change the actual code. You can see the workflows lint failing. You can use make lint to validate this (once you install the deps).

The goal of this is to get consistent formatting in the SQL strings, not change any actual go code, which is already being linted and automatically formatted.

Feel free to re-add me to the PR once you fix that.

[ammiranda]

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

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]

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]

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

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

[bplunkett-stripe]

Nice seems good to merge in. In terms of the script, I think it should first be converted to golang to standardize the # of languages we have the repo.

In terms of aligning with the other linters, it probably makes sense to have a --fix flag, i.e., a validation mode vs a write mode.

Alternatively, you can have it always write to the files, and we can add it to the code_gen step.

[ammiranda]

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]

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

• Re-writes the acceptance tests to have tab-less multi-line strings
• Preferably has a--fix option, such that there is basically a validation/dry-run mode and a write mode
• Is called from the makefile lint and lint_fix targets (like go_lint, sql_lint, etc)

[bplunkett-stripe]

@ammiranda Are you planning on working on a go-version of your script as described above?

[ammiranda]

@bplunkett-stripe sure! I can create the issue and you can update it and add details as needed if that works for you.

[bplunkett-stripe]

@bplunkett-stripe sure! I can create the issue and you can update it and add details as needed if that works for you.

Sounds good with me, thanks!

[bplunkett-stripe]

import os
import in_place

dir_path = "./internal/migration_acceptance_tests"

filenames = os.listdir(dir_path)

for filename in filenames:
    path = os.path.join(dir_path, filename)
    if os.path.isdir(path):
        continue
    with in_place.InPlace(path) as fp:
        in_sql_string = False
        for line in fp:
            if in_sql_string and '`' not in line:
                new_line = line.replace("\t", "    ")
                fp.write(new_line)
            else:
                fp.write(line)
            if '`' in line:
                in_sql_string = not in_sql_string

Updated place-holder script to account for dirs