-
Notifications
You must be signed in to change notification settings - Fork 38
Port to Windows #482
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
Port to Windows #482
Conversation
7f3ef83
to
1d2f2ca
Compare
039cd4a
to
e545217
Compare
Only 97 failed tests under Windows now. A lot look to be some kind of filesystem-tree breakage. |
Hey @techtonik want to review this for me? Give me a 👍 and I'll merge it; then we can squish windows bugs. |
@@ -120,7 +121,7 @@ def get_wildleaf_fallback(): | |||
wild_nonleaf_ns = [ n for n in maybe_wild_nodes if not is_leaf_node(n) ] | |||
|
|||
# store all the fallback possibilities | |||
remaining = reduce(traverse, nodepath[depth:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where was traverse
coming from? Is it used elsewhere? Should this PR delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's passed in as a param, and is used correctly ( to join fs paths ) in other places. This spot was actually rejoining URL path parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha.
DESCRIPTION
Instead of importing this module directly, import os and refer to
this module as os.path. The "os.path" name is an alias for this
module on Posix systems; on other systems (e.g. Mac, Windows),
os.path provides the same operations in a manner specific to that
platform, and is an alias to another module (e.g. macpath, ntpath).
Some of this can actually be useful on non-Posix systems too, e.g.
for manipulation of the pathname component of URLs.
Where did pylint sneak in? From the Appveyor template we started with? Let's turn it off. |
... or teach it about our coding style. |
Coverage report looks very broken as well. Sums to 0%. |
Why are the skipped tests skipping? Iirc they are skipping because of an "is on Windows" condition. If so let's review and potentially remove the skip. |
I think pylint and coverage are because we're running the same target (analyse) here as we do on Travis. Maybe it should be just 'test' instead? |
|
I am seeing pylint on Travis for Python 2.7 (and it looks like the version of pylint we're using uses set literal syntax not supported under 2.6). Coverage is fine under both 2.6 and 2.7. Out of scope for this PR. |
We should test on Python 2.6 at Appveyor. |
And still, the coverage report on AppVeyor sums to 0%, whereas the coverage report for the same Python version on Travis sums to 62%. @pjz If you don't want to fix that here then we should at least reticket so we remember to check again after we squish all the Windows bugs (which could be causing the discrepancy, I suppose). |
I got back from offline. Let me see what is happening here. |
@techtonik Welcome back! :-) In my view we are trying to bring AppVeyor at least up to par with what we have at Travis. TODOs:
|
I reconfigured our AppVeyor account to represent Gratipay and not whit537, using the @gratipay-gremlin account to link with GitHub. |
I'm applying |
I'd first improve visibility of the issue by placing badges in #484, but I need API key for that. |
Do you? See #484 (comment). |
So should I turn off pylint and code coverage here? |
@pjz The tasks I see for this PR are at #482 (comment). I don't mind about pylint since it's on Travis and I think we are trying to achieve parity with Travis in order to merge this PR. |
I'm going to go ahead and fix the README thing here too, since it's not worth doing until this is in anyway. |
What license is aspen under anyway? I don't see it listed anywhere. |
ah, there it is. MIT. |
dc29727
to
ebf718a
Compare
Okay, ready for review again. Also turned README.txt into README.rst so that I could then add an AppVeyor shield to it. Also added the shield to the aspen.io site. |
Ping @whit537 |
Tests passing on Mac OS. |
Coverage is still borked at 0%. Wanna fix that or reticket, @pjz? |
reticket, pls. |
Oh, and I verified the skips are legit: 'Windows file locking makes these fail' |
Reticketed #488. |
Cool. ;-) |
Congratulations @pjz @techtonik. Aspen is (re-)ported to Windows, now with continuous integration to ensure we keep it that way! 👯 |
👍 |
Appveyor is now running the code under windows... and finding platform-specific bugs.