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

PSL: Implementing map field for @default attribute on SQL Server #2156

Merged
merged 12 commits into from Aug 17, 2021

Conversation

pimeys
Copy link
Contributor

@pimeys pimeys commented Aug 16, 2021

@pimeys pimeys added this to the 2.30.0 milestone Aug 16, 2021
@pimeys pimeys force-pushed the sql-server/default-constraint-name branch from cbee5e3 to 7be2e7b Compare August 16, 2021 20:49
Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

Last comment is the important one. Depending on how much time we have, I think we should maybe look at attributes.rs and course correct.

libs/datamodel/connectors/dml/src/default_value.rs Outdated Show resolved Hide resolved
}

db_name.map(|cow| cow.to_string())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I already had concerns and re-read the code for constraint names validation in attributes.rs, now I'm certain we should change the approach asap, we're going down the wrong path (this code does not belong here). Do you have time for a call today so we can sketch the better approach? I'd also be fine merging this and correcting later, but it's only getting more expensive to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll have a call later. Let's do this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a call, we as a team decided to redo this after GA.

@pimeys pimeys merged commit bea9c1f into master Aug 17, 2021
@pimeys pimeys deleted the sql-server/default-constraint-name branch August 17, 2021 14:00
Weakky pushed a commit that referenced this pull request Aug 23, 2021
…2156)

* Make `DefaultValue` a struct

* Actually migrate named defaults

* Start testing

* More tests

* Rest of the test, ME/IE + PSL lower

* Make `DefaultValue` properties private.

* clippy

* clippy

* Remove DRY parts

* Fix error string

* Use db_names to define the default constraint name

* clippy
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 Default Constraint Name to PSL for SqlServer
3 participants