Skip to content

Conversation

@euresti
Copy link
Contributor

@euresti euresti commented Jan 25, 2019

Hi. I started testing this against our codebase and noticed some errors.

Copy link
Contributor

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks for PR! It looks indeed like an oversight, @bryanforbes could you please double-check?

@ilevkivskyi
Copy link
Contributor

Btw, @euresti it would be great if you could add a test or couple for this (e.g. just a model with one foreign key constraint that accepts a list of columns). Note: our test cases aren't run, just type-checked with mypy (they are very similar to mypy's test suite).

@bryanforbes
Copy link
Contributor

Yes, this was an oversight on my part. Good find @euresti!

@ilevkivskyi does he need to sign a CLA?

@ilevkivskyi
Copy link
Contributor

Oh, yes, indeed I completely forgot about CLA, @euresti could you please sign it?

@euresti
Copy link
Contributor Author

euresti commented Jan 29, 2019

Done!

@ilevkivskyi
Copy link
Contributor

Thanks for signing CLA! Are you going to add the test as I suggested above? (If not, I will merge this anyway, but it is always better with a test.)

@euresti
Copy link
Contributor Author

euresti commented Feb 1, 2019

I'll look into a test. I was testing this on our code base and found a couple of other issues with the stubs. I'll file Issues for those since I'm not sure what the correct solution to some of those are.

@euresti
Copy link
Contributor Author

euresti commented Feb 1, 2019

Test has been added.

@ilevkivskyi
Copy link
Contributor

Thanks!

@ilevkivskyi ilevkivskyi merged commit 66c0312 into dropbox:master Feb 1, 2019
@ilevkivskyi
Copy link
Contributor

I was testing this on our code base and found a couple of other issues with the stubs. I'll file Issues for those since I'm not sure what the correct solution to some of those are.

OK, sure.

@euresti euresti deleted the sequence_type branch February 1, 2019 17:15
kespindler pushed a commit to januarytech/sqlalchemy-stubs that referenced this pull request Feb 2, 2019
@MattF-NSIDC MattF-NSIDC mentioned this pull request Apr 11, 2019
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