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.

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

Confirm drop by prefix, add --continue #140

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Confirm drop by prefix, add --continue #140

merged 3 commits into from
Oct 24, 2023

Conversation

dogversioning
Copy link
Contributor

@dogversioning dogversioning commented Oct 23, 2023

This PR makes the following changes:

  • Asks for confirmation when using the --prefix argument Tech Debt: Add list of tables and confirmation to manual prefix dropping #129
  • Adds a --continue param for specifying a point in the manifest's list of SQL files to continue a build from (useful for debugging errors that occur at near the end of a build)
  • As a result of doing this, cleaned up some places where clean wasn't quite wired into reading a prefix directly from a manifest

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added
  • Run pylint if you're making changes beyond adding studies
  • Update template repo if there are changes to study configuration

mock_cursor.__iter__.return_value = [[query_res]]
parser = StudyManifestParser("./tests/test_data/study_valid/")
tables = parser.clean_study(mock_cursor, schema, verbose, prefix=prefix)
with mock.patch.object(builtins, "input", lambda _: confirm):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun

cumulus_library/study_parser.py Outdated Show resolved Hide resolved
cumulus_library/study_parser.py Outdated Show resolved Hide resolved
print(continue_from.replace(".sql", ""))
print(file.replace(".sql", ""))
if continue_from.replace(".sql", "") == file.replace(".sql", ""):
sql_files = sql_files[sql_files.index(file) :]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could save yourself an index() call by doing for i, file in enumerate(sql_files): above, but 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, i dig it

Comment on lines 105 to 106
if not match_found:
sys.exit(f"No tables matching '{continue_from}' found")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You ready for this Python nonsense? Just do else: and skip match_found. For/else lets you run code if the for loop never broke.

i.e.:

for file in sql_files:
    if continue_from.replace(".sql", "") == file.replace(".sql", ""):
        sql_files = sql_files[sql_files.index(file) :]
        break
else:
    sys.exit(f"No tables matching '{continue_from}' found")

Alternatively, you could also do return sql_files[sql_files.index(file) :] and unconditionally exit after the loop.

Or keep it! Whatever works for your sense of code cleanliness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking the for\else approach, just in case there's other post loop stuff that might end up here in the future.

build.add_argument(
"--continue",
dest="continue_from",
help=(argparse.SUPPRESS),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the parens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy pasted from elsewhere - why i did that in the first place, ¯\_(ツ)_/¯

@dogversioning dogversioning merged commit be07f8a into main Oct 24, 2023
3 checks passed
@dogversioning dogversioning deleted the mg/cli_continue branch October 24, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants