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.

Sign up for GitHub

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

Jump to bottom

V2: move convertToBlob from core/io to io-utils for coherence & practicality #509

Merged
merged 11 commits into from
Feb 1, 2020
Merged

V2: move convertToBlob from core/io to io-utils for coherence & practicality #509

merged 11 commits into from
Feb 1, 2020

Conversation

kaosat-dev
Copy link
Contributor

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests?

@z3dev : this is a small one, but long overdue, and fixes a few issues I had locally

@kaosat-dev
refactor(io): moved bulk of core/io to packages/io (utils, and io its…
4d9f7a3 
…elf)

* replaced prepareOutput + convertToBlob (used in almost all packages)
with solidsAsBlob() utility
* lots of related tweaks & cleanups
@kaosat-dev
refactor(io): removed a lot of redundant io code (& related) & update…
8630a3b 
…d imports
@kaosat-dev
refactor(array-utils): moved array utils out of core , to :
229f9fe 
* have a more specific package (do one thing & do it well)
* resolve circular dependency problems between packages
@kaosat-dev
refactor(core): removed obsolete module
43ff4cf
@kaosat-dev kaosat-dev requested a review from z3dev January 30, 2020 09:52
@kaosat-dev
Copy link
Contributor Author

@z3dev this is ready for review :
a bit of background :

  • it moves the bulk of core/io (particularly convertToBlob & related code) to io itself
  • since it created circular dependencies (core<->io<->core), I extracted all array utilities into a new package, that also allowed to get rid of a ton of duplicate code !
  • related cleanups

z3dev
z3dev reviewed Jan 30, 2020
View reviewed changes
Copy link
Member

@z3dev z3dev left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a general question... can the movement from ‘core to IO’ be split from the new array package (I like this)?

packages/utils/array-utils/index.js Outdated
@@ -30,4 +30,20 @@ const nth = (index, data) => {
return data[index]
}

module.exports = {toArray, head, flatten, nth}
/* function flatten (array) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the flatten in modeling, which is recursive. I think that’s a better implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, btw I did not touch the array things in modeling because there are other people doing changes in modeling, and we can always make modeling import array utils.
Btw about recursive vs non recursive : perhaps we should have both: I remember there was a case where a single level of flattening was expected (I cannot recall the exact location, sorry :( )

We could provide a 'flattenDeep' or 'flattenNested' or a parameter ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just go with the recursive version for flatten() as that's what we really want for handling lists of objects coming into API functions.

If we need a function to 'clean' an array then let's create and name appropriately.

@z3dev
Copy link
Member

z3dev commented Jan 30, 2020

I found another package using a specific toArray() function... svg-deserializer

@kaosat-dev
Copy link
Contributor Author

kaosat-dev commented Jan 30, 2020 •
edited
Loading

can the movement from ‘core to IO’ be split from the new array package (I like this)?

No, sadly, because as stated above just the move results in circular dependencies.

@kaosat-dev
Copy link
Contributor Author

I found another package using a specific toArray() function... svg-deserializer

good catch ! will update

@kaosat-dev
Copy link
Contributor Author

@z3dev should be all good

@z3dev
Copy link
Member

z3dev commented Jan 31, 2020

@z3dev should be all good

I didn’t have time to review in full. I’ll finish tomorrow morning, which should be while you are sleeping. 😃

z3dev
z3dev reviewed Feb 1, 2020
View reviewed changes
packages/io/io/package.json Show resolved Hide resolved
packages/utils/array-utils/index.js Outdated
@@ -30,4 +30,20 @@ const nth = (index, data) => {
return data[index]
}

module.exports = {toArray, head, flatten, nth}
/* function flatten (array) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just go with the recursive version for flatten() as that's what we really want for handling lists of objects coming into API functions.

If we need a function to 'clean' an array then let's create and name appropriately.

@z3dev
Copy link
Member

z3dev commented Feb 1, 2020

@kaosat-dev nice last commit. 👍🏻

You can merge. I’m very satisfied with this.

@kaosat-dev
Copy link
Contributor Author

thanks @z3dev ! Merging !

@kaosat-dev kaosat-dev merged commit 3e4316a into V2 Feb 1, 2020
@kaosat-dev kaosat-dev deleted the v2-io-blob branch February 6, 2020 09:12
@kaosat-dev kaosat-dev mentioned this pull request Feb 10, 2020
Closed
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@z3dev z3dev z3dev left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kaosat-dev @z3dev