byuccl / spydrnet

A flexible framework for analyzing and transforming FPGA netlists. Official repository.
https://byuccl.github.io/spydrnet
BSD 3-Clause "New" or "Revised" License
86 stars 20 forks source link

parse should work with Pathlib.Path #195

Closed jgoeders closed 1 year ago

jgoeders commented 1 year ago

spydernet.parse throws a AttributeError: 'PosixPath' object has no attribute 'readable' if you provide a PosixPath pathlib.Path object.

emonlux commented 1 year ago

I believe that I was able to recreate the same error. Including .name at the end of the command (e.g. pathlib.Path('...').name) should fix the issue. Is this something that you want added into the parser in cases where the user does not include .name?

jgoeders commented 1 year ago

Yes, .name works because it returns a string; however, the parse API should also accept a pathlib.Path object as that is the standard way of providing file paths in Python.

emonlux commented 1 year ago

I added on the next release branch a flag to pass to the parser if pathlib.Path is being used. The flag is path_used Does this fix the issue for you?

jacobdbrown4 commented 1 year ago

Instead of having to pass a parameter to the parser, can we just check what the passed object is? If it's a string then don't do anything, and if it's a pathlib.Path object then get the filename string from it and continue on.

jgoeders commented 1 year ago

I agree with @jacobdbrown4. It's odd to have a flag. It's expected that python libraries that accept a file path will just work with pathlib.Path objects.

if it's a pathlib.Path object then get the filename string from it and continue on

Yes, you could add something like this to the start of the function:

filename = str(filename)

However, you should really be keeping the path as a pathlib.Path object internally, and more correctly, if passed a string, convert it to pathlib.Path object right away.

Right now it looks like the code is using os.path everywhere. This is the old way of doing paths in Python. The code should be using pathlib for all path handling and manipulations. It's much more readable and the modern Python way of dealing with paths.

jacobdbrown4 commented 1 year ago

Thanks for the info. We should probably update SpyDrNet to use pathlib like you were saying.

emonlux commented 1 year ago

@jgoeders

@jacobdbrown4 and I have added the following functionality to the parser that is on the most recent release.

if it's a pathlib.Path object then get the filename string from it and continue on

Does this fix the issue for you?

Also, I have started looking into updating the code from os.path to pathlib. I have noticed different types of paths e.g. PosixPath, PurePosixPath, PurePath, and Path as well as path for windows. Is there a standard type of path that should be used?

emonlux commented 1 year ago

Remaining os.path in code

jacobdbrown4 commented 1 year ago

I have gone through and updated everything that uses os.path to use pathlib. I believe it is all working okay. My work is on the next_release branch.

I might create some tests that use pathlib paths to make sure parsing and composing works okay with that.

jacobdbrown4 commented 1 year ago

This is in next release branch and waiting to be released.