-
Notifications
You must be signed in to change notification settings - Fork 2
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
A bunch of small improvements while working on the wizard back and resume #556
Conversation
Review changes with SemanticDiff. Analyzed 6 of 6 files. Overall, the semantic diff is 21% smaller than the GitHub diff.
|
832a2ba
to
9f19ff5
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #556 +/- ##
==========================================
+ Coverage 74.64% 74.85% +0.20%
==========================================
Files 46 46
Lines 3132 3142 +10
Branches 510 510
==========================================
+ Hits 2338 2352 +14
+ Misses 693 691 -2
+ Partials 101 99 -2 ☔ View full report in Codecov by Sentry. |
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.
I would prefer not to import readalongs.
I have a question about can_make_dir()
.
try: | ||
from readalongs.portable_tempfile import ( | ||
PortableNamedTemporaryFile as NamedTemporaryFile, | ||
) | ||
except ImportError: | ||
from tempfile import NamedTemporaryFile | ||
|
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.
Is there a way not to require readalongs? Is EV already dependent on readalongs? Wouldn't it be wiser to have that readalongs.protable_temfile
be a new library that both EV and readalongs could import?
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.
We don't depend on readalongs yet, and with the try/except we still don't really, which is why I did not add it to requirements.
Also, when we do #439, we will then really add the readalongs dependency to support that export feature, which is in part why I thought I could slide this weird thing in.
def add_step(self, step: Step, parent: Step): | ||
"""Insert a step in the specified position in the tour. | ||
|
||
Args: | ||
step: The step to add | ||
parent: The parent to add the step to | ||
""" |
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.
👍🏼 Thank you for adding documentation for the methods.
while not d.exists() and d not in (Path("/"), Path(""), Path(".")): | ||
dirs_to_make.append(d) | ||
d = d.parent |
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.
Will this survive path=="/somewhere_exist/doesntexist/doesnexists/../../a/b/c
?
I guess it depends on d.parent
.
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.
Yes, this is exactly the use case I was working to support, and the reason I did this patch. Previously, validation would have left /somewhere_exist/doesntexist/doesnexists/../../a/b
on the filesystem, now this while loop will add all the components to dirs_to_make
and stop at /somewhere_exist
, since it exists.
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.
And the finally block at the end of this function will remove them all so we leave no traces behind just for trying to validate that we can create this deep directory.
For dev't purposes, we want to be able to skip it, but let's force the dev to set an env variable to do so, instead of using a warning that might be missed if someone does not look at the logs.
- refactor: test_configs use readalongs PortableNamedTmpFile allows more unit tests to pass on Windows. - make test_utils.py compatible with Windows by expecting either windows or posix paths
also: - document the methods in the wizard's Tour class - remove the unused child_index arg to add_step
9f19ff5
to
6d7a98f
Compare
PR Goal?
I made these changes while working on the wizard's back and resume functions, but I want to PR them separately because they will be confusing to review as part of that PR.
Fixes?
Feedback sought?
sanity check
Priority?
medium: these changes will be required for the wizard improvement PR
Tests added?
yes
How to test?
Confidence?
high
Version change?
no
Related PRs?
n/a