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

Default value migration #625

Closed
fishead opened this issue Mar 6, 2022 · 5 comments
Closed

Default value migration #625

fishead opened this issue Mar 6, 2022 · 5 comments

Comments

@fishead
Copy link
Contributor

fishead commented Mar 6, 2022

Great project.

Default value of string like field can be empty string "" or "null", So why check it like this?

// ValidString reports if the given string is not null and valid.
func ValidString(s sql.NullString) bool {
return s.Valid && s.String != "" && strings.ToLower(s.String) != "null"
}

I'm not an expert of this, any special consideration?

edit:
After test with my own project and as @a8m mentioned in comment, the function is not the problem.

@a8m
Copy link
Member

a8m commented Mar 6, 2022

Hey @fishead 👋

It should be fixed. Let me know if you want to contribute and I can guide you, or I can do it later today 🙏

@fishead
Copy link
Contributor Author

fishead commented Mar 6, 2022

I'm glad to make a PR.

@a8m
Copy link
Member

a8m commented Mar 6, 2022

Here's where we handle the default values:
https://github.com/ariga/atlas/blob/master/sql/mysql/inspect.go#L240
https://github.com/ariga/atlas/blob/master/sql/postgres/inspect.go#L181
https://github.com/ariga/atlas/blob/master/sql/sqlite/inspect.go#L141

We should use defaults.Valid instead of sqlx.ValidString. Just note that I added a commit (sqltest) in #622 to fix testing for it, so rebase and it's merged.

@fishead
Copy link
Contributor Author

fishead commented Mar 6, 2022

@a8m patch is here #626, just let me known if it's not appropriate.
You are so friendly in the ent community.

a8m pushed a commit that referenced this issue Mar 6, 2022
* fix: Default value migration #625

* fix: MariaDB case
@a8m
Copy link
Member

a8m commented Mar 6, 2022

Fixed with #626. Thanks a lot @fishead

@a8m a8m closed this as completed Mar 6, 2022
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

No branches or pull requests

2 participants