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

Added a way to check if a row has a value for a column #654

Closed
wants to merge 1 commit into from

Conversation

GregOriol
Copy link

This PR adds a way to check if a result row has a value for a column.

This is useful when you make requests on sqlite databases that may not have some columns (ex: doing a select * and a previous version of the database didn't have the column). Currently, there is no way to know if the column is available, and in this case using the getter on a Row makes a fatalError that can't be caught. With this addition, it is possible to call hasColumn on a Row to check it before.

@jberkel
Copy link
Collaborator

jberkel commented Sep 16, 2017

I think the right way to address this is to avoid raising fatalError in the column accessor, and instead use a specific error ColumnNotFound or similar. Then your code can handle this specific case.

@nickmshelley
Copy link
Collaborator

This type of problem is largely mitigated by the new RowCursor implemented in #647. We've been using a fork containing that change in our apps for a while now and it seems to work well. I haven't heard any feedback on it yet.

@nickmshelley
Copy link
Collaborator

Actually maybe it's #649 that fixes this specific issue. I've tried to make the library safer to use by throwing errors instead of crashing in many places.

Technically I'm a committer so I can merge those myself, but I trust @jberkel and @stephencelis to make/keep this library great more than I trust myself.

@jberkel
Copy link
Collaborator

jberkel commented Sep 16, 2017

@nickmshelley ok, I'm going to add a fix similar to #649 for now.
Overhauling the whole api to fix the error handling is probably a task for 0.12.

@jberkel
Copy link
Collaborator

jberkel commented Sep 16, 2017

@GregOriol with 0.11.4 you'll be able to handle the noSuchColumn error, when using get (subscripts will still crash)

@jberkel jberkel closed this Sep 16, 2017
@GregOriol
Copy link
Author

GregOriol commented Sep 17, 2017

@jberkel @nickmshelley Thanks for the update

I agree there are many ways to do it and don't think mine here would be the definitive one. However, I'd have a few comments on this choice and #649:

  • I like the idea of having different kinds of errors, this is very nice
  • It could be good to have a way to "check" instead of "wait for an error"; while both have the same result overall, it sometimes makes code more easy to read with an "if", than with a "try/catch"
  • Having a breaking change in a minor version (0.11.4) doesn't seem a good idea... while a limited use-case, it is still something that could have been used by some devs, better wait a 0.12 to change existing methods
  • Also, minor one, I tend to think that having a different behavior between a getter and subscript could be quite confusing; or should be documented carefully

@jberkel
Copy link
Collaborator

jberkel commented Sep 18, 2017

It is a breaking chance, but I think the large majority of users will access row data via one of the subscripts, which are unchanged. The get method isn't really mentioned in the docs. And if it does break, it can be fixed easily.

Regarding consistency: subscripts cannot throw errors, so I don't think this is a problem. It should probably be mentioned though.

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.

3 participants