Skip to content
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

os.scandir #11365

Merged
merged 7 commits into from
Oct 13, 2018
Merged

os.scandir #11365

merged 7 commits into from
Oct 13, 2018

Conversation

kd2718
Copy link
Contributor

@kd2718 kd2718 commented Oct 6, 2018

FIX #9914
I added os.scandir in two locations. I also updated how osm.py checked for executable isexec. I took advantage of some type hinting. I don't think type hinting is used elsewhere in the code, so I can remove this if needed.

@kd2718
Copy link
Contributor Author

kd2718 commented Oct 6, 2018

I forgot... Typing was added in 3.6. This causes the build for 3.5 to fail. I will update

@kd2718
Copy link
Contributor Author

kd2718 commented Oct 8, 2018

read for review

self.execre = re.compile(r'(.*)\.(%s)$' % winext,re.IGNORECASE)

# call up the chain
super(OSMagics, self).__init__(shell=shell, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

This can be just super() since IPython requires Python 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. I will update this

@@ -481,6 +520,7 @@ def dhist(self, parameter_s=''):

dh = self.shell.user_ns['_dh']
if parameter_s:
args = []
Copy link
Member

Choose a reason for hiding this comment

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

This empty args is never used

Copy link
Contributor Author

@kd2718 kd2718 Oct 10, 2018

Choose a reason for hiding this comment

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

Yes, it's overwritten on line 525 and the return on line 528 prevents a case where args is not in scope.

However, the linter wasn't fully aware of this and got worried that args may not be implemented at the time of use. Looking forward, if the except is changed to handle a specific exception, rather than a generic exception, then this will prevent args from being out of scope.

This may be over protective, so I can still remove the line in question if preferred.

IPython/core/magics/osm.py Outdated Show resolved Hide resolved
except OSError:
continue

# use with notation for python 3.6 onward
Copy link
Member

Choose a reason for hiding this comment

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

I guess you mean types annotations that you removed , I thing that can be trimmed now.

@Carreau
Copy link
Member

Carreau commented Oct 12, 2018

Not all your machines are setup with the same email, and/or github does not have all your emails so some commits do not appear as yours.

I believe 3.5 have function signature types, so these would have been ok to leave in I think.

There is 2 small leftover 1 debug statement and 1 comment. And we're good to merge IMHO.

@Carreau Carreau added this to the 7.1 milestone Oct 12, 2018
@Carreau
Copy link
Member

Carreau commented Oct 12, 2018

@meeseeksdev tag hacktoberfest

@lumberbot-app lumberbot-app bot added the Hacktoberfest you want to participate to hacktoberfest ? Here is an easy issue. label Oct 12, 2018
@Carreau
Copy link
Member

Carreau commented Oct 13, 2018

Thanks.

@Carreau Carreau merged commit 32eec53 into ipython:master Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest you want to participate to hacktoberfest ? Here is an easy issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants