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

Add informative notes to invariant function arguments #3411

Merged
merged 31 commits into from Aug 19, 2017

Conversation

Projects
None yet
4 participants
@quartox
Contributor

quartox commented May 22, 2017

Fixes #1115 and #3352

Note that the check.test files must now use out since there is a # in the documentation url, which breaks the # E: syntax.

@gnprice gnprice self-assigned this May 22, 2017

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 22, 2017

Collaborator

Thanks for this fix!

Note that the check.test files must now use out since there is a # in the documentation url, which breaks the # E: syntax.

Hmm. Kind of a shame to move all of these to the out form -- both because it's a little harder to read in itself (as the reader now loses the convenience of having the message right there on the line it's about), and because it makes the diff bigger and therefore harder to understand. I suspect this issue wouldn't be hard to fix -- want to do that first?

Collaborator

gnprice commented May 22, 2017

Thanks for this fix!

Note that the check.test files must now use out since there is a # in the documentation url, which breaks the # E: syntax.

Hmm. Kind of a shame to move all of these to the out form -- both because it's a little harder to read in itself (as the reader now loses the convenience of having the message right there on the line it's about), and because it makes the diff bigger and therefore harder to understand. I suspect this issue wouldn't be hard to fix -- want to do that first?

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 22, 2017

Collaborator

Alternatively -- these URLs are already pretty long. Maybe move this docs section to its own page? :-)

Collaborator

gnprice commented May 22, 2017

Alternatively -- these URLs are already pretty long. Maybe move this docs section to its own page? :-)

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox May 22, 2017

Contributor

Moving to a new page would do it. The current section is very far down the page, so difficult to expect the user to find it easily.
An alternative would be allow some form of escaping the # character in the check*.test files.

Contributor

quartox commented May 22, 2017

Moving to a new page would do it. The current section is very far down the page, so difficult to expect the user to find it easily.
An alternative would be allow some form of escaping the # character in the check*.test files.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 22, 2017

Member
Member

gvanrossum commented May 22, 2017

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 23, 2017

Collaborator

Even better than making a new page (though we can also do that too): I think we should make a redirect on RTD from some nice, shorter path (like variance?) to wherever we have it documented (like common_issues.html#invariance-vs-covariance).

That'd shorten the URL, get the # out -- and also give us the flexibility to move and rename that bit of the docs over time and preserve the links in existing versions of mypy.

Would want to put a comment in the docs source saying that the redirect should be updated when moved or renamed. We'd want a comment like that with or without the redirect, if we're going to link to it from error messages.

Collaborator

gnprice commented May 23, 2017

Even better than making a new page (though we can also do that too): I think we should make a redirect on RTD from some nice, shorter path (like variance?) to wherever we have it documented (like common_issues.html#invariance-vs-covariance).

That'd shorten the URL, get the # out -- and also give us the flexibility to move and rename that bit of the docs over time and preserve the links in existing versions of mypy.

Would want to put a comment in the docs source saying that the redirect should be updated when moved or renamed. We'd want a comment like that with or without the redirect, if we're going to link to it from error messages.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum May 23, 2017

Member

The RTD redirect sounds too fancy (and too RTD-specific). Let's just move the explanation of *variance to its own page, to be named variance.html. It's worth it.

Member

gvanrossum commented May 23, 2017

The RTD redirect sounds too fancy (and too RTD-specific). Let's just move the explanation of *variance to its own page, to be named variance.html. It's worth it.

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 23, 2017

Collaborator

I just made such a redirect a few minutes ago as an experiment -- if I'm understanding the admin UI and the docs right, it ought to show up at http://mypy.readthedocs.io/en/latest/variance .

That URL 404s right now. Maybe it takes some time to propagate? Not super confidence-inspiring; there's no mention of that that I can see in the interface or the docs.

Collaborator

gnprice commented May 23, 2017

I just made such a redirect a few minutes ago as an experiment -- if I'm understanding the admin UI and the docs right, it ought to show up at http://mypy.readthedocs.io/en/latest/variance .

That URL 404s right now. Maybe it takes some time to propagate? Not super confidence-inspiring; there's no mention of that that I can see in the interface or the docs.

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 23, 2017

Collaborator

I'm slightly concerned, but only slightly, about it being RTD-specific -- I assume that basically any reasonable way of hosting web pages now and in the future, especially one oriented to documentation, has some way of redirecting one page's URL to another. I do wish that the way to express the redirect were in the docs' source tree rather than in an RTD admin UI on the web.

Collaborator

gnprice commented May 23, 2017

I'm slightly concerned, but only slightly, about it being RTD-specific -- I assume that basically any reasonable way of hosting web pages now and in the future, especially one oriented to documentation, has some way of redirecting one page's URL to another. I do wish that the way to express the redirect were in the docs' source tree rather than in an RTD admin UI on the web.

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox May 23, 2017

Contributor

Successfully splitting check*.test files on ' # ' (instead of '#') with all existing tests passing. This seems like an appropriate choice to me, but judge for yourself. Let me know if you settle on re-direct vs new html and we can fix the url.

Contributor

quartox commented May 23, 2017

Successfully splitting check*.test files on ' # ' (instead of '#') with all existing tests passing. This seems like an appropriate choice to me, but judge for yourself. Let me know if you settle on re-direct vs new html and we can fix the url.

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 23, 2017

Collaborator

OK, it's been at least 10 minutes and that URL doesn't work, and also after some more discussion I'm convinced we don't want to rely on RTD's implementation of anything fancy for us.

Let's just move that section to a new page variance -- that'll be a very sensible page for us to have forever. Let's move that in this same PR.

The change to split on # is a nice cleanup in any case, I think; I'd be glad to see it as a separate tiny PR.

Collaborator

gnprice commented May 23, 2017

OK, it's been at least 10 minutes and that URL doesn't work, and also after some more discussion I'm convinced we don't want to rely on RTD's implementation of anything fancy for us.

Let's just move that section to a new page variance -- that'll be a very sensible page for us to have forever. Let's move that in this same PR.

The change to split on # is a nice cleanup in any case, I think; I'd be glad to see it as a separate tiny PR.

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox May 23, 2017

Contributor

I just did a straight move from to a new file. I have no experience with sphinx so don't know how to test that it builds the docs correctly.

Contributor

quartox commented May 23, 2017

I just did a straight move from to a new file. I have no experience with sphinx so don't know how to test that it builds the docs correctly.

quartox added some commits May 23, 2017

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 23, 2017

Collaborator

(Update FTR for whatever it means about RTD: http://mypy.readthedocs.io/en/latest/variance is still a 404 page.)

Collaborator

gnprice commented May 23, 2017

(Update FTR for whatever it means about RTD: http://mypy.readthedocs.io/en/latest/variance is still a 404 page.)

@gnprice

This comment has been minimized.

Show comment
Hide comment
@gnprice

gnprice May 23, 2017

Collaborator

@quartox No experience needed! Take a look at docs/README.md for how to build and view changes you make in the docs.

Collaborator

gnprice commented May 23, 2017

@quartox No experience needed! Take a look at docs/README.md for how to build and view changes you make in the docs.

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox
Contributor

quartox commented May 23, 2017

quartox added some commits May 24, 2017

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 19, 2017

Collaborator

@quartox It looks like several comments are still not addressed, please ping me back when you are ready.

Collaborator

ilevkivskyi commented Aug 19, 2017

@quartox It looks like several comments are still not addressed, please ping me back when you are ready.

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox Aug 19, 2017

Contributor

@ilevkivskyi I have updated to address comments, but I am using the CI to check tests for me since my machine is very underpowered. I plan to address failing tests and we should be ready for more review after the tests are passing.

Contributor

quartox commented Aug 19, 2017

@ilevkivskyi I have updated to address comments, but I am using the CI to check tests for me since my machine is very underpowered. I plan to address failing tests and we should be ready for more review after the tests are passing.

@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox Aug 19, 2017

Contributor

@ilevkivskyi the function definition now runs over 100 characters, what is the preferred way to move part to the next line?

Contributor

quartox commented Aug 19, 2017

@ilevkivskyi the function definition now runs over 100 characters, what is the preferred way to move part to the next line?

Show outdated Hide outdated mypy/messages.py
@ilevkivskyi

OK, hopefully two last comments.

Show outdated Hide outdated mypy/messages.py
Show outdated Hide outdated test-data/unit/check-varargs.test
@quartox

This comment has been minimized.

Show comment
Hide comment
@quartox

quartox Aug 19, 2017

Contributor

@ilevkivskyi Excellent, I just pushed the fixes. Thank you very much for your help. I know it was probably frustrating, but I learned a lot from the experience. Let me know if there are any more comments.

Contributor

quartox commented Aug 19, 2017

@ilevkivskyi Excellent, I just pushed the fixes. Thank you very much for your help. I know it was probably frustrating, but I learned a lot from the experience. Let me know if there are any more comments.

@ilevkivskyi

Thanks! LGTM now, will merge when tests pass.

@ilevkivskyi ilevkivskyi merged commit 2534523 into python:master Aug 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment