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

DOC: Handle whitespace in pathnames #20880

Merged
merged 3 commits into from
Sep 26, 2018
Merged

DOC: Handle whitespace in pathnames #20880

merged 3 commits into from
Sep 26, 2018

Conversation

thiviyanT
Copy link
Contributor

@thiviyanT thiviyanT commented Apr 30, 2018

Fixed make.py - Now should be able to run make.py in paths with whitespaces (#20862)

@codecov
Copy link

codecov bot commented Apr 30, 2018

Codecov Report

Merging #20880 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20880      +/-   ##
==========================================
+ Coverage   92.19%   92.19%   +<.01%     
==========================================
  Files         169      169              
  Lines       50821    50825       +4     
==========================================
+ Hits        46852    46857       +5     
+ Misses       3969     3968       -1
Flag Coverage Δ
#multiple 90.6% <ø> (ø) ⬆️
#single 42.37% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/window.py 96.4% <0%> (+0.11%) ⬆️
pandas/core/groupby/base.py 91.83% <0%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a393675...a919eff. Read the comment docs.

@gfyoung gfyoung added the Docs label May 8, 2018
@gfyoung gfyoung changed the title DOC: Fix whitespace in pathnames issue by wrapping paths with quotes … DOC: Fix whitespace in pathnames May 8, 2018
@gfyoung gfyoung changed the title DOC: Fix whitespace in pathnames DOC: Handle whitespace in pathnames May 8, 2018
@jreback jreback added this to the 0.24.0 milestone Jun 19, 2018
@jreback
Copy link
Contributor

jreback commented Jun 19, 2018

@datapythonista @jorisvandenbossche

how does this look?

@datapythonista
Copy link
Member

I was taking a look at it yesterday, as I'm not sure in windows the single quotes will handle backslashes well. But unrelated to the ticket I'm getting a segmentation fault when running sphinx-built. Will comment on the ticket with feedback when I manage to test it.

@jorisvandenbossche
Copy link
Member

Can't test on Windows at the moment, but for linux this patch works nicely and fixes the problem for me.

@datapythonista
Copy link
Member

@ThiviyanThanapalasingam seems like in Windows the paths need to be escaped with double quotes (the backlash is interpreted as an escape character like in \n).

Besides that, I personally find it's not very readable to use backslashes to escape the quotes. Meaning that instead of '\'{}\'' I think it'd be more clear to use "'{}'". Or now that we need to change to double quotes, the opposite: '"{}"'.

Also, I see that there are several paths that are built like source/*.rst (instead of os.path.join('source', '*.rst'). I couldn't try, but I guess sphinx is failing to exclude these files like in unix systems. Do we want to open an issue for making the script work in Windows @jreback?

@datapythonista datapythonista self-assigned this Jul 22, 2018
@datapythonista
Copy link
Member

@ThiviyanThanapalasingam do you have time to update the PR based on the feedback I provided?

@thiviyanT
Copy link
Contributor Author

Yes, I can do that. @datapythonista Have you opened another issue for making the script work in Windows?

@datapythonista
Copy link
Member

No, I think it should be just the one you opened.

@jreback
Copy link
Contributor

jreback commented Sep 25, 2018

@datapythonista ?

@datapythonista
Copy link
Member

@ThiviyanThanapalasingam can you update the PR based on the previous feedback?

@pep8speaks
Copy link

Hello @ThiviyanThanapalasingam! Thanks for updating the PR.

@datapythonista
Copy link
Member

Made the updates to this PR. But seems like there is a new unrelated problem when the path contains whitespaces: #22836

I think that can be addressed in a separate PR. I would merge this on green, but happy to close this and take care of both problems in a new PR.

@jorisvandenbossche jorisvandenbossche merged commit 7343fd3 into pandas-dev:master Sep 26, 2018
@jorisvandenbossche
Copy link
Member

Let's do that in a separate PR

@ThiviyanThanapalasingam thanks for the original PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants