Skip to content

Conversation

@coldnebo
Copy link
Contributor

Fixes #330. it looks like there is more than one way to collect column names, but I'm not sure what 'real' column names are in the context of schema_statements.rb#L253. Is my fix ok, or does it mask other issues?

@coldnebo coldnebo changed the title fix for #330 prevent downstream nil exception when view_definition is nil. May 19, 2014
annaswims added a commit that referenced this pull request Aug 27, 2014
prevent downstream nil exception when view_definition is nil.
@annaswims annaswims merged commit bda0dac into rails-sqlserver:master Aug 27, 2014
@metaskills
Copy link
Member

Not sure if guarding against these are a good idea. See:

Not sure, but the general history I remember is that being silent on this core issue has caused more grieve than it saved.

@coldnebo
Copy link
Contributor Author

The warn behavior in the fix for #73 is unchanged by this fix. However, if you are saying that it's better to warn the user during find() rather than silencing the downstream error, I agree.

It might be better to duplicate the warning here too.

coldnebo added a commit to coldnebo/activerecord-sqlserver-adapter that referenced this pull request Aug 27, 2014
@coldnebo
Copy link
Contributor Author

What do you think of this approach?

@metaskills
Copy link
Member

It might be better to duplicate the warning here too.

Yea, that might be the best way. Raising an error in both places caused issues with those that could not set permissions due to legacy and/or overzealous DBA's. Likewise, being silent has caused errors and confusion too. So connecting up a second warn that pointed to the first might be best.

coldnebo added a commit to coldnebo/activerecord-sqlserver-adapter that referenced this pull request Aug 27, 2014
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.

view_information should raise instead of warn

3 participants