Base reader, refactor and pathlib support

[tetov]

This is a new PR with the changes from #348 but cleaned up.

@gonzalocasas latest code review will be copied over to this PR.

What type of change is this?

• New feature in a backwards-compatible manner.

Checklist

1. Add the change to the CHANGELOG.md file in the Unreleased section under the most fitting heading: Added, Changed, Removed.
2. Run all tests on your computer (i.e. invoke test).
3. Add unit tests (especially important for algorithm implementations).

[tetov]

Checks fail because I test using a file that is not yet present in compas/data..

[tomvanmele]

there is a lot going on in this PR.
perhaps we should focus on the base reader first?

[gonzalocasas]

Added some early feedback, but I assume you're still working on it because it's marked as draft, right?

[tetov]

@gonzalocasas

Added some early feedback, but I assume you're still working on it because it's marked as draft, right?

I'll flag you when another review is needed, I need to move the tests and and resolve your comments above.

There are some open questions above I'd like answered as well :).

• Keep file signature check?
• Rename or remove count of vertices, faces, edges?
• New name for BaseReader.read()
• Good rewording of error message?

Todo list for me:

• Move and rework tests in test_mesh that really belong to the file readers
• Test if ply.py will ignore user-defined elements
• Follow up weird edit, (just check git blame)
• Download files to tmp when given URL and iterate over contents
• Docstrings and comments

[tetov]

Yeah, totally agree. iterlines and iterchunks?

…

On Mon, Dec 9, 2019 at 21:46, Gonzalo Casas ***@***.***> wrote: @gonzalocasas commented on this pull request. --------------------------------------------------------------- In [src/compas/files/base_reader.py](#418 (comment)): > + from urllib2 import urlopen + + +class BaseReader(object): + """Base class containing file reading functions for file extension specific readers + + Attributes + ---------- + location : str or pathlib object + Path or URL to the file + """ + def __init__(self, location): + self.is_url = False + self.location = self.check_location(location) + + def read(self, mode='text'): Instead of having one method that does two things and receives a mode to if on it, why not have two methods properly named: read_line() and read_bytes() and ditch the mode parameter? — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#418?email_source=notifications&email_token=ADRRKRO5JWKL57LUMZ4BGFTQX2VDXA5CNFSM4JWDOKAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOPVDDY#discussion_r355675136), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADRRKRJA6L6BAXMWBODMOMDQX2VDXANCNFSM4JWDOKAA).

[tetov]

Or read_line & read_chunks. Sorry missed last part of the comment.

…

On Mon, Dec 9, 2019 at 21:46, Gonzalo Casas ***@***.***> wrote: @gonzalocasas commented on this pull request. --------------------------------------------------------------- In [src/compas/files/base_reader.py](#418 (comment)): > + from urllib2 import urlopen + + +class BaseReader(object): + """Base class containing file reading functions for file extension specific readers + + Attributes + ---------- + location : str or pathlib object + Path or URL to the file + """ + def __init__(self, location): + self.is_url = False + self.location = self.check_location(location) + + def read(self, mode='text'): Instead of having one method that does two things and receives a mode to if on it, why not have two methods properly named: read_line() and read_bytes() and ditch the mode parameter? — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#418?email_source=notifications&email_token=ADRRKRO5JWKL57LUMZ4BGFTQX2VDXA5CNFSM4JWDOKAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOPVDDY#discussion_r355675136), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADRRKRJA6L6BAXMWBODMOMDQX2VDXANCNFSM4JWDOKAA).

[tomvanmele]

i think this PR is too big. perhaps we should split off the base reader and get that sorted first. also, perhaps this is useful for binary/not binary https://github.com/audreyr/binaryornot

[tetov]

I've moved my other changes to another branch on my fork to make the scope of this PR more limited.

I'll work through the remaining pointers for base_reader and update the PR (tonight hopefully).

To add to this PR:

• Download file to tmp and start iterating over lines
• Tests
• Docstrings
• Binary detection using library @brgcode suggested (https://github.com/audreyr/binaryornot)

Future PR's:

• Update file reader modules to use base_reader (one PR per module)

[tomvanmele]

test fails on number of lines check of ply_ascii

[tetov]

test fails on number of lines check of ply_ascii

Working on it :)

[tetov]

Is it time to squash the commits and remove the WIP status?

…

On Tue, Dec 17, 2019 at 22:16, Tom Van Mele ***@***.***> wrote: @brgcode commented on this pull request. --------------------------------------------------------------- In [src/compas/files/base_reader.py](#418 (comment)): > @@ -129,8 +139,8 @@ def iter_lines(self): for line in fo: yield line.rstrip() - def iter_chunks(self, chunk_size=1024): - """Yields lines from local binary files + def iter_chunks(self, chunk_size=4096): no leave it as it is — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#418?email_source=notifications&email_token=ADRRKRMCJ7IEJAFM7K2SN2TQZE6SDA5CNFSM4JWDOKAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPRBWKA#discussion_r359032261), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ADRRKRO5E3R3RGVTTDP4DADQZE6SDANCNFSM4JWDOKAA).

[tomvanmele]

True if correct file signature for file type is found in file, or if file type has no signature.

[tomvanmele]

nearly there

[tomvanmele]

last changes and then we can merge

[tetov]

last changes and then we can merge

I got a warning from autosummary because it couldn't find BaseReader. I guess it's because it's not imported in init.py. I removed the entry.

Also, I tried interactive rebase but since I merged with master earlier I'm worried about squashing other commits as well. Maybe you could use Github's solution with my last commit msg?