File reading function moved to new parent class & pathlib support #348

tetov · 2019-10-06T16:25:07Z

BaseReader.read_from_location returns an iterable for lines in file.

File format	ASCII	Binary	Skeleton?	BaseReader implemented
`amf`	xml		x	x
`dxf`	x	x	x	x
`las`		x	x	x
`obj`	x			x
`off`	x			x
`ply`	x	x		For ASCII
`stl`	x	x		Not yet
`urdf`	xml			Not yet
`xml`	x			Needs whole file to parse?
`xml_cli`	x			Needs whole file to parse?

What type of change is this?

New feature in a backwards-compatible manner.

Checklist

Add the change to the CHANGELOG.md file in the Unreleased section under the most fitting heading: Added, Changed, Removed.
Run all tests on your computer (i.e. invoke test).
If you add new functions/classes, check that:
1. Are available on a second-level import, e.g. compas.datastructures.Mesh.
2. Add unit tests (especially important for algorithm implementations).

`BaseReader.read_from_location` returns an iterable for lines in file. | File format | ASCII | Binary | Skeleton? | BaseReader implemented | | ----------- | ----- | ------ | --------- | ------------------------- | | `amf` | xml | | x | x | | `dxf` | x | x | x | x | | `las` | | x | x | x | | `obj` | x | | | x | | `off` | x | | | x | | `ply` | x | x | | For ASCII | | `stl` | x | x | | Not yet | | `urdef` | xml | | | Not yet | | `xml` | x | | | Needs whole file to parse?| | `xml_cli` | x | | | Needs whole file to parse?|

tetov · 2019-10-06T16:39:14Z

I will look into adding the same functionality for binary files, either keep this PR open until that is done or merge this and I'll submit a new PR later on.

Questions

Would you like me to move the tests in tests/compas/files/test_stl.py to tests/compas/datastructures/test_mesh.py? Could be fitting to do it in this PR.
Would you like me to put the BaseReader class in a file with another name, e.g. _utils.py?
I'm guessing that iterating over lines won't work for the XML reader, am I right in that assumption?

moved into corresponding modules instead of keeping them in separate files

Small spelling mistake in docstring

tetov · 2019-10-08T07:40:11Z

I updated questions in above comment. :)

updated docstring of 3 plane intersection

Licini

LGTM

Licini · 2019-10-08T09:14:24Z

I will look into adding the same functionality for binary files, either keep this PR open until that is done or merge this and I'll submit a new PR later on.

Questions

Would you like me to move the tests in tests/compas/files/test_stl.py to tests/compas/datastructures/test_mesh.py? Could be fitting to do it in this PR.

Would you like me to put the BaseReader class in a file with another name, e.g. _utils.py?

I'm guessing that iterating over lines won't work for the XML reader, am I right in that assumption?

My thoughts on questions : )

Yep, let's merge the content of test_stl.py into test_from_stl in test_mesh.py.
It's better to keep as it is for clarity.
agree, since it's marked-up language. It would be nice to have an mechanism to throw an error if user tries to load xml in this fashion

tetov · 2019-10-08T09:20:44Z

agree, since it's marked-up language. It would be nice to have an mechanism to throw an error if user tries to load xml in this fashion

There's usually checks in the different modules, e.g. ply.py will throw an error if first line is not 'ply'. It could of course be added to the BaseReader as a general check if you want :).

Licini · 2019-10-09T09:22:04Z

agree, since it's marked-up language. It would be nice to have an mechanism to throw an error if user tries to load xml in this fashion

There's usually checks in the different modules, e.g. ply.py will throw an error if first line is not 'ply'. It could of course be added to the BaseReader as a general check if you want :).

That's should be fine then, as long as user be informed the reason of breaking

Update `Frame`, `Transformation`

src/compas/files/ply.py

tetov · 2019-10-23T16:09:35Z

Per IRL discussion with @brgcode, binary ply has not been implemented, so that test will be commented out.

The code needs to be cleaned up, travis errors fixed but it's slowly getting there.

`BaseReader.read_from_location` returns an iterable for lines in file. | File format | ASCII | Binary | Skeleton? | BaseReader implemented | | ----------- | ----- | ------ | --------- | ------------------------- | | `amf` | xml | | x | x | | `dxf` | x | x | x | x | | `las` | | x | x | x | | `obj` | x | | | x | | `off` | x | | | x | | `ply` | x | x | | For ASCII | | `stl` | x | x | | Not yet | | `urdef` | xml | | | Not yet | | `xml` | x | | | Needs whole file to parse?| | `xml_cli` | x | | | Needs whole file to parse?|

…b_support

tetov · 2019-10-28T11:15:20Z

Oh and sorry about the amount of commits. Didn’t know a rebase would show up like that on GitHub. I’ll merge next time.

tomvanmele · 2019-12-04T14:45:28Z

@gonzalocasas can we merge this?

tetov · 2019-12-04T14:47:13Z

@gonzalocasas, @brgcode
#348 (comment)

gonzalocasas

I tried to review this in detail, but the rebase thing gets a lot in the way. There are a few things that are not clear to me, some naming things that should change before merging, the removal of tests, and other remarks; but really, the length of the current PR makes it tricky to review. If merging master back into this branch does not get rid of all the spurious diffs, then I would suggest you create a new clean branch, and cherry pick your changes on top of a clean status.

.bumpversion.cfg

CHANGELOG.md

docs/conf.py

setup.py

src/compas/__init__.py

gonzalocasas · 2019-12-04T15:46:24Z

src/compas/files/off.py

-        header = next(self.content)
-        if not header.lower() == 'off':
-            return
+            raise Exception('Import failed')


Import failed is not very clear, we're not really importing, we're opening files (which will then might be imported into something), so I would be more explicit and limited to this module's functionality in the error message.

Moved to #418.

src/compas/files/ply.py

src/compas/geometry/basic.py

tests/compas/files/test_stl.py

tetov · 2019-12-04T16:28:00Z

I’ll open a fresh one (and go through the comments).

tetov · 2019-12-04T17:20:15Z

I replied to some. The ones I didn’t reply to I agree to completly.

I’ll move over your comments to new PR. Thanks for reviewing.

tetov

All unresolved issues moved to #418.

Licini and others added 10 commits September 18, 2019 15:13

separate constructors from helpers

9ee04dc

Merge branch 'compas_rhino_constructors'

e5167b7

move rhino modifiers and selectors

dcd098a

Update volmeshselector.py

3d3dcaa

Update meshmodifier.py

42f6b9e

Merge remote-tracking branch 'upstream/master'

31605ca

adding change basis

4f25091

update represent local/global with change_basis

d8dd4ed

small update on quaternion

a1484ff

Licini and others added 6 commits October 7, 2019 16:21

re-adjusting selector and modifier wrappers

4f1a6ae

moved into corresponding modules instead of keeping them in separate files

deprecation warning for helper

e7cd0bb

Merge remote-tracking branch 'upstream/master'

71f6e0f

Small spelling mistake in docstring

26e0c3d

Merge pull request #351 from tetov/patch-1

ec909df

Small spelling mistake in docstring

Keep BaseReader more private

4086c73

tomvanmele and others added 4 commits October 8, 2019 09:40

fixed coloring bug in mesh face drawing

a83c0e8

updated docstring of 3 plane intersection

Merge branch 'master' of https://github.com/compas-dev/compas

fc224b9

return quaternion

1e92bd0

returning vector from axis angle vector

0ba2c02

Licini approved these changes Oct 8, 2019

View reviewed changes

romanarust added 2 commits October 8, 2019 11:50

adding unitized and scaled to vector

7ceaac3

fixing returns and docstrings

e240212

romanarust added 2 commits October 9, 2019 11:59

added transform_frames, fix dehomogenize_numpy

49fbf5b

adding correct_axis_vectors

60a985f

romanarust and others added 8 commits October 18, 2019 07:51

addind functions to __all__

ce8b53d

Merge pull request #357 from compas-dev/update-frame-and-transformation

6ec660b

Update `Frame`, `Transformation`

Bump version to 0.9.0

032a1f8

Prepare changelog for next release

27b5ebf

Update changelog

cd4f4f9

fixed line point transformations

9f963b0

cleaning up unnecessary imports

839ea72

Refactor and pathlib implemented, issues with ply and urllib in python2

1a05357

tetov commented Oct 23, 2019

View reviewed changes

src/compas/files/ply.py Show resolved Hide resolved

tetov added 2 commits October 23, 2019 22:35

Removed test for binary ply-files & fixes

6aae814

Removed test for binary ply-files & fixes 2

a20d830

tetov requested a review from gonzalocasas October 23, 2019 21:05

tetov added 7 commits October 23, 2019 23:06

Keep BaseReader more private

a4c07f0

Draft: need to check CI

4f4a2fa

Refactor and pathlib implemented, issues with ply and urllib in python2

1a3d58c

Removed test for binary ply-files & fixes

8cae052

Removed test for binary ply-files & fixes 2

87d4070

Merge branch 'pathlib_support' of github.com:tetov/compas into pathli…

477a12e

…b_support

gonzalocasas requested changes Dec 4, 2019

View reviewed changes

tetov mentioned this pull request Dec 5, 2019

Base reader, refactor and pathlib support #418

Merged

4 tasks

tetov commented Dec 5, 2019

View reviewed changes

tetov closed this Dec 5, 2019

tetov deleted the pathlib_support branch December 5, 2019 22:52

File reading function moved to new parent class & pathlib support #348

File reading function moved to new parent class & pathlib support #348

Uh oh!

Conversation

tetov commented Oct 6, 2019 • edited by gonzalocasas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of change is this?

Checklist

Uh oh!

tetov commented Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tetov commented Oct 8, 2019

Uh oh!

Licini left a comment

Choose a reason for hiding this comment

Uh oh!

Licini commented Oct 8, 2019

Uh oh!

tetov commented Oct 8, 2019

Uh oh!

Licini commented Oct 9, 2019

Uh oh!

Uh oh!

tetov commented Oct 23, 2019

Uh oh!

tetov commented Oct 28, 2019

Uh oh!

tomvanmele commented Dec 4, 2019

Uh oh!

tetov commented Dec 4, 2019

Uh oh!

gonzalocasas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gonzalocasas Dec 4, 2019

Choose a reason for hiding this comment

Uh oh!

tetov Dec 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tetov commented Dec 4, 2019

Uh oh!

tetov commented Dec 4, 2019

Uh oh!

tetov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tetov commented Oct 6, 2019 •

edited by gonzalocasas

Loading

tetov commented Oct 6, 2019 •

edited

Loading