Replace sys.path to pathlib.Path #95
Conversation
sbillinge
left a comment
sbillinge
left a comment
There was a problem hiding this comment.
not quite, please see if you can figure it out. Don't guess, try and figure out the logic.
Also, need a CR at the end of the doc.yml. You can see this by reviewing your own PR.....
doc/manual/source/conf.py
Outdated
| import os | ||
| sys.path.insert(0, os.path.abspath('../..')) | ||
| sys.path.insert(0, os.path.abspath('../../src')) | ||
| pathlib.Path.insert(0, os.path.abspath('../..')) |
There was a problem hiding this comment.
this isn't how you do it. Please try and figure out how to do it differently using pathlib.
check that the docs build locally before pushing so we don't use CI credits for debugging but only testing debugged code. Also, there is still an os.path in there .....
There was a problem hiding this comment.
maybe ask chatgpt what that old syntax is doing so you understand that, then read a bit about pathlib so you know hte background and then trh and figure out how to do it with pathlib (and in the end ask chatgpt how how it would do it)..... (giving away my secrets here...don't let chatgpt take over, but get help)
There was a problem hiding this comment.
Remember to also refactor these ../.. into ../../..
There was a problem hiding this comment.
so we will keep sys.path, there is no pathlib replacement for that, it contains the list of directories the system looks in in to find executables, but os.path.abspath will go to str(Path('../..').resolve()) (but get the right number of parent directories so everything works. and I think we need the str())
There was a problem hiding this comment.
You still require sys.path as that gives the interpreter knowledge of where to look for modules. The update is to remove uses of os. Though generally fine to use, os handles paths as strings, while Pathlib creates a platform-specific object with some additional functionalities. Probably the most useful is glob if you have worked with the * operator on bash.
doc/manual/source/conf.py
Outdated
| import os | ||
| sys.path.insert(0, os.path.abspath('../..')) | ||
| sys.path.insert(0, os.path.abspath('../../src')) | ||
| pathlib.Path.insert(0, os.path.abspath('../..')) |
There was a problem hiding this comment.
maybe ask chatgpt what that old syntax is doing so you understand that, then read a bit about pathlib so you know hte background and then trh and figure out how to do it with pathlib (and in the end ask chatgpt how how it would do it)..... (giving away my secrets here...don't let chatgpt take over, but get help)
There was a problem hiding this comment.
Need to refactor <../../tutorial/tutorialData.zip> to <../../../tutorial/tutorialData.zip>
doc/manual/source/conf.py
Outdated
| import os | ||
| sys.path.insert(0, os.path.abspath('../..')) | ||
| sys.path.insert(0, os.path.abspath('../../src')) | ||
| pathlib.Path.insert(0, os.path.abspath('../..')) |
There was a problem hiding this comment.
so we will keep sys.path, there is no pathlib replacement for that, it contains the list of directories the system looks in in to find executables, but os.path.abspath will go to str(Path('../..').resolve()) (but get the right number of parent directories so everything works. and I think we need the str())
|
@sbillinge I saw .parents[] in pathlib. Will this not work? |
|
Parents and resolve do different things. If x is a PosixPath (on Mac/Linux) or WindowsPath (on... Windows), then x.resolve() gives the absolute path (starting from root) as well as resolving symbolic links (which indeed makes it more general). What we have given to the program is a relative path as |
|
It is nice if the code is more readable as this makes it more easily
maintained, so my preference would be `Path( '../..').resolve()` rather
than something like `Path( '.' ).resolve().parents[2]`
…On Tue, May 21, 2024 at 5:19 PM Sparky ***@***.***> wrote:
Parents and resolve do different things. If x is a PosixPath (on
Mac/Linux) or WindowsPath (on... Windows), then x.resolve() gives the
absolute path (starting from root) as well as resolving symbolic links
(which indeed makes it more general). What we have given to the program is
a relative path as .. means the outer directory of this current
directory. On the other hand x.parents returns a list of outer directories,
and you would have to manually recombine all these directories to
reconstruct the absolute path.
—
Reply to this email directly, view it on GitHub
<#95 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAOWUMWNSMGTKNX5NQ2PFTZDO265AVCNFSM6AAAAABICCSV3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGQ3DAOBRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Simon Billinge
Professor,
Department of Applied Physics and Applied Mathematics
Columbia University
|
|
@cadenmyers13 thanks for this, great job! For future reference, there is way too much on this PR. This would have been better as two or even three PRs. It is best to make multiple branches (off a fully synced main each time) and only do one thing on each PR. That makes things WAY easier to review and get merged. You can (should) have mulitple PRs open from different branches at any given time. Also, only merge commits from other branches when absolutely necessary. If we design things right we can keep things of each branch sufficiently separated that they can be worked on independently. Not always, but as long as things are getting merged quickly (because they are small) we rarely run into problems (another reason to do small granular PRs) I am not sure if everything is quite right on this, but if not we can open new PRs to fix any problems. Thanks so much, great job! I know it is a steep learning curve! Thanks again. I know it is a learning experience. |
3 participants
No description provided.