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

✨ add --ignore-errors flag #87

Closed
wants to merge 11 commits into from
Closed

Conversation

CWBudde
Copy link

@CWBudde CWBudde commented Aug 19, 2023

Along with the --ignore-errors flag I also implemented a couple of other minor fixes. In the end it led to the fact that only 4 errors (due to bad design in the original sqlite3 database) needed to get fixed manually.

In the end I'm not 100% sure that the current code (after several revisions) is the best it could be. So I'd be happy about a critical review. In case the changes are too much, I could split this PR.

return date.fromisoformat(value.decode())
decoded_value: str = value.decode()
if " " in decoded_value:
decoded_value = decoded_value.split(" ")[0]
Copy link
Owner

@techouse techouse Aug 19, 2023

Choose a reason for hiding this comment

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

This smells to me like you're trying to mangle a datetime into a date.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. I'm sorry that I forgot to remove this piece and reverted that code. It was a bad and legacy database I tried to convert.

@@ -251,7 +252,7 @@ def _translate_type_from_sqlite_to_mysql(self, column_type: str) -> str:
full_column_type: str = column_type.upper()
match: t.Optional[t.Match[str]] = self._valid_column_type(column_type)
if not match:
raise ValueError("Invalid column_type!")
raise ValueError("Invalid column_type: " + column_type)
Copy link
Owner

Choose a reason for hiding this comment

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

Please use an f-string here, i.e.

raise ValueError(f"Invalid column_type: {column_type}")

auto_increment="AUTO_INCREMENT" if auto_increment else "",
default="DEFAULT " + column["dflt_value"]
if column["dflt_value"] and column_type not in MYSQL_COLUMN_TYPES_WITHOUT_DEFAULT and not auto_increment
Copy link
Owner

Choose a reason for hiding this comment

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

So here you want to completely ignore the fact that a field can not have a default type? Why?

Copy link
Author

Choose a reason for hiding this comment

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

Good find. That code should not have been deleted. I was mangling around with the code and I guess after a few iterations this piece got lost.

@techouse techouse added the enhancement New feature or request label Aug 19, 2023
@techouse techouse self-assigned this Aug 19, 2023
sql += " `{name}` {type} {notnull} {default} {auto_increment}, ".format(
name=mysql_safe_name,
type=column_type,
notnull="NOT NULL" if column["notnull"] or column["pk"] else "NULL",
notnull="NOT NULL" if not_null else "NULL",
Copy link
Owner

@techouse techouse Aug 19, 2023

Choose a reason for hiding this comment

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

I prefer the explicit condition for readability.

Copy link
Author

Choose a reason for hiding this comment

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

OK

@techouse
Copy link
Owner

techouse commented Aug 19, 2023

In the end I'm not 100% sure that the current code (after several revisions) is the best it could be.

That's why I made the PR into a draft for now. 🤞Take your time with this. If this repo has taught me anything it's that one ounce of prevention is worth a pound of cure. 😊

I've left a few comments. Can you please take a look at them.

Also, please add unit tests. 🙏

@techouse techouse marked this pull request as draft August 19, 2023 08:57
@techouse techouse changed the title Added --ignore-errors flag ✨ add --ignore-errors flag Aug 19, 2023
@techouse techouse assigned CWBudde and unassigned techouse Oct 15, 2023
@techouse techouse linked an issue Nov 15, 2023 that may be closed by this pull request
@techouse techouse closed this Apr 16, 2024
@techouse
Copy link
Owner

@CWBudde I closed this PR because it hasn't seen any activity in over 6 months. Please feel free to reopen if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip errors
2 participants