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

Remove pg_dump dependency #32

Merged
merged 1 commit into from
Sep 6, 2019

Conversation

yudppp
Copy link
Contributor

@yudppp yudppp commented Sep 4, 2019

I checked Drop pg_dump dependency issue. #10.
Easy to implement Drop pg_dump dependency and Support SERIAL together.

@k0kubun
Please check my plan is correct.

TODO

  • Support SERIAL -> I will fix other PR(this PR is Drop pg_dump dependenc only)
  • Add test code
  • Support multi primary key

@k0kubun
Copy link
Collaborator

k0kubun commented Sep 5, 2019

Wow, that looks great. I think it's okay to go with this once the existing integration tests pass. I appreciate your effort working on this.

@yudppp yudppp force-pushed the feature/remove_pg_dump_dependency branch 4 times, most recently from b53f211 to 0b8b365 Compare September 5, 2019 15:01
@yudppp yudppp changed the title [WIP] Remove pg_dump dependency Remove pg_dump dependency Sep 5, 2019
@yudppp yudppp marked this pull request as ready for review September 5, 2019 15:03
@yudppp yudppp changed the title Remove pg_dump dependency [WIP]Remove pg_dump dependency Sep 5, 2019
@yudppp yudppp force-pushed the feature/remove_pg_dump_dependency branch from 385554b to ab0b2fb Compare September 5, 2019 15:52
@yudppp yudppp changed the title [WIP]Remove pg_dump dependency Remove pg_dump dependency Sep 5, 2019
@yudppp yudppp force-pushed the feature/remove_pg_dump_dependency branch from ab0b2fb to ee15b0f Compare September 5, 2019 15:57
@yudppp
Copy link
Contributor Author

yudppp commented Sep 5, 2019

@k0kubun
I Finished.And passed all existing integration tests

@k0kubun
Copy link
Collaborator

k0kubun commented Sep 6, 2019

Sounds good. Thanks a lot!

@k0kubun k0kubun merged commit cc95a71 into sqldef:master Sep 6, 2019
Comment on lines +168 to +174
if colDefault != nil || isPK {
if isPK {
col.IsPrimaryKey = true
} else {
col.Default = *colDefault
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, did you have any reason why this wasn't:

		if isPK {
			col.IsPrimaryKey = true
		}
		if colDefault != nil {
			col.Default = *colDefault
		}

More specifically, did you intend to skip setting col.Default when colDefault != nil and isPk are true?

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