-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
Migrate hazmat to faked explicit importing and reexporting, to aid static analysis (issue #542) #753
Migrate hazmat to faked explicit importing and reexporting, to aid static analysis (issue #542) #753
Conversation
Codecov Report
@@ Coverage Diff @@
## master #753 +/- ##
==========================================
+ Coverage 99.01% 99.01% +<.01%
==========================================
Files 96 96
Lines 11677 11682 +5
Branches 830 828 -2
==========================================
+ Hits 11562 11567 +5
Misses 94 94
Partials 21 21
Continue to review full report at Codecov.
|
It seems the try / except worked with |
Before I continue on hazmat, I'd thought we'll discuss the |
I'm not sure what you mean here but it sounds like you might be confused about something? I don't know why |
I think the simplest way to handle this is a few from ._core import (
# all the symbols that are available universally
)
try:
from ._core import (
# all the Windows symbols
)
except ImportError:
pass
try:
from ._core import (
# all the epoll symbols
)
except ImportError:
pass
try:
from ._core import (
# all the kqueue symbols
)
except ImportError:
pass (Actually, I guess there might not be any epoll-specific symbols, so we might be able to leave that section out?) Anyway, the point of doing it this way is that it's pretty simple, and I think should let us stop touching |
Yes, I had an error in the tests and it took me a while to find out this was the reason. As you see it already works but some symbols are still missing. Or we could move the code into the functions and have the class using the functions. |
Please do not modify any file inside the |
Yes, right, I was just looking a little a head a little bit. Got a full code completion this way and wanted to see if this works. There is always |
4c885cb
to
21cd622
Compare
(Merged #756, which causes a conflict in trio/tests/test_exports.py as expected, sorry!) |
No worries, Nathaniel already warned me of that. I'll merge locally, that should do it. |
@njsmith The comment to
in Sorry for nitpicking 😄 |
Productive weekend. Some symbols from kqueue, epoll and the magic |
This is a little different from the So, I think the way I'd do this is:
# Unix-specific symbols
try:
from ._core import (
wait_writable,
wait_readable,
notify_fd_close,
)
except ImportError:
pass
# Kqueue-specific symbols
try:
from ._core import (
current_kqueue,
monitor_kevent,
wait_kevent,
)
except ImportError:
pass
# Windows symbols
# ...
|
Hi @njsmith I'll check on this over the next day, but as far as I remember that did not work. The
that will do the job, as during import time they all get overwritten again. But static analysis does not get that the functions are getting overwritten and will just display the stubs. |
|
@njsmith It's currently not obvious from the track if all tasks have been finished. I had some really busy weeks. A little refresh could help :-) |
@jmfrank63 I think this comment is the thing to look at, still: #753 (comment) |
You also need to enable the hazmat test in |
Haha, yes, just found this out and saw your comment after the refresh. |
e4f0630
to
e7ddafd
Compare
Hi @njsmith Tried to split the try except block which worked but I had to keep the dynamic globals update as without it the build failed. So there is no point in splitting only makes the code longer and less readable. I think we should leave it as it is now. Works and is readable. |
Can you show the code you tried and the test results? Even if this is true I think it's still important to understand why... |
It looks like this is the commit: ffdf069 |
Oh, I guess the appveyor tests failed too: https://ci.appveyor.com/project/njsmith/trio/builds/20995101/job/n10octd21tdj1ris?fullLog=true That failure is because you put |
That’s easy to check., I’ll try again.
…On Fri, 14 Dec 2018 at 10:11, Nathaniel J. Smith ***@***.***> wrote:
Oh, I guess the appveyor tests failed too:
https://ci.appveyor.com/project/njsmith/trio/builds/20995101/job/n10octd21tdj1ris?fullLog=true
That failure is because you put wait_socket_readable, wait_socket_writable,
and notify_socket_close in the "unix-only" section, but they should
actually be in the "always available" section. (As noted in item 3 here
<#753 (comment)>.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#753 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC8oHmtVbYFAZdZ21-px3MBKSF_AHL9Lks5u43lNgaJpZM4X94q6>
.
|
@njsmith Sorry I overlooked part two, should have looked more closely. |
Yep, looks good to me, thanks! We don't have a JIRA or anything besides Github's built-in issues/PR stuff... if you want a way to keep track of what you plan to do and what you haven't done yet, then one option is to make a "checklist". This is a cute trick github supports, where you can write a regular comment with bullet points like As for what to do next, I think this comment is currently the most complete checklist for what we need to do for #542: #542 (comment) Merging this is an exciting milestone – I think jedi can now complete all top-level public symbols in Trio 🎉 ✨ Unfortunately this also means that the next steps are more complicated :-) |
Continuing towards finalisation of #542
We do a try / except import of our symbols to aid static analysis tools to pick them up. Then we update the list dynamically to have python get the correct symbols.