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

fix: values in compile_alter_sql #885

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Conversation

Kyrela
Copy link
Contributor

@Kyrela Kyrela commented Jun 19, 2024

fix #884

note that the change line 343 doesn't change anything as the function isn't used, but could be useful if this behavior is changed.

Works with the example given in the issue.

Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

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

just needs a test

@Kyrela
Copy link
Contributor Author

Kyrela commented Jun 19, 2024

Should I edit BaseConnection.py to remove the additional blank line?

@josephmancuso
Copy link
Member

Should I edit BaseConnection.py to remove the additional blank line?

idk what youre talking about but sure i think

@Kyrela
Copy link
Contributor Author

Kyrela commented Jun 19, 2024

I'm talking about the lint results.
image
No link with my PR, but as the linter is triggered by it, I guess I could fix it?

@josephmancuso
Copy link
Member

oh, yes please

@josephmancuso
Copy link
Member

adding tests are pretty simple if you could add one for enum

@Kyrela
Copy link
Contributor Author

Kyrela commented Jun 19, 2024

adding tests are pretty simple if you could add one for enum

Oh, sure, was thinking about it.

@Kyrela
Copy link
Contributor Author

Kyrela commented Jun 19, 2024

Not sure about the quality of my tests tbh, but I added a test for the bug referenced in issue #885 and tests for changing an enum or adding it in a new table. If these are good, should I add similar ones for the other sql dialects?

@josephmancuso
Copy link
Member

Not sure about the quality of my tests tbh, but I added a test for the bug referenced in issue #885 and tests for changing an enum or adding it in a new table. If these are good, should I add similar ones for the other sql dialects?

yes please, code looks good 👍

@Kyrela Kyrela marked this pull request as draft June 20, 2024 12:41
@Kyrela Kyrela marked this pull request as ready for review June 20, 2024 17:17
@Kyrela
Copy link
Contributor Author

Kyrela commented Jul 7, 2024

Hello, any update?

@josephmancuso josephmancuso merged commit 5649282 into MasoniteFramework:2.0 Jul 30, 2024
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.

Add Enum column in migration not working (MySQL)
2 participants