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.

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

feat(core): zip.select and assorted predicates #113

Merged
merged 18 commits into from
Jan 12, 2019
Merged

Conversation

andon
Copy link
Member

@andon andon commented Aug 26, 2018

Implements the function

const select: (...arguments) => location => Iterable<locations>

The function is in curried form. It expects a variable arg-set that is used to select/filter starting from the current location.

Each argument can be:

  • a 'string' which is interpreted as descending into a property. The property should have an element (or child elements)
  • an array of strings which is interpreted as the predicate ofKind
  • a function returning a list of locations (selector or motion function)
  • a predicate function used to filter out nodes

Also implements the functions that can me used in the select function, in some of the following categories:

  • selectors
    • ancestors an iterable of the location's ancestors; nearest parent first
    • descendants, an iterable of all descendants, depth-first, right-to-left
    • children an iterable of the location's children
    • childrenAt(key) an iterable of the location's children for the specified key
  • predicates
    • ofKind(kind) a predicate function which will return true if the location points to an element the given kind

Closes #53

@andon andon requested a review from ognen August 26, 2018 15:29
@andon andon added discussion A discussion issue or PR WIP Work in progress: a PR is labeled WIP is not to be merged labels Aug 26, 2018
@andon andon changed the title Feat/zip select feat(core): zip.select and assorted predicates Aug 27, 2018
@andon andon removed the discussion A discussion issue or PR label Aug 27, 2018
packages/core/src/zip/__tests__/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/__tests__/select.js Show resolved Hide resolved
packages/core/src/zip/__tests__/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/__tests__/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/__tests__/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/__tests__/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/__tests__/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/select.js Outdated Show resolved Hide resolved
@andon andon changed the base branch from feat/registry-package to master August 31, 2018 07:21
@andon

This comment has been minimized.

@coveralls
Copy link

coveralls commented Sep 23, 2018

Pull Request Test Coverage Report for Build 420

  • 69 of 70 (98.57%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 81.716%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/src/zip/skele/selector.js 21 22 95.45%
Totals Coverage Status
Change from base Build 417: 0.8%
Covered Lines: 1204
Relevant Lines: 1413

💛 - Coveralls

@andon andon requested review from AleksandarDembovski and removed request for bPopovska December 22, 2018 08:11
@andon andon added core and removed WIP Work in progress: a PR is labeled WIP is not to be merged labels Dec 22, 2018
@andon andon self-assigned this Dec 22, 2018
packages/core/src/zip/__tests__/motion.js Outdated Show resolved Hide resolved
packages/core/src/zip/selector.js Outdated Show resolved Hide resolved
packages/core/src/zip/selector.js Outdated Show resolved Hide resolved
packages/core/src/zip/selector.js Outdated Show resolved Hide resolved
Copy link
Member

@ognen ognen left a comment

Choose a reason for hiding this comment

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

Well, look at this:

> function* four() {
... console.log('up to 1')
... yield 1
... console.log('up to 2')
... yield 2
... console.log('up to 3')
... yield 3
... console.log('over')
... }
undefined
> var x = four()
undefined
> for (var i of x) {
... console.log('got', i)
... }
up to 1
got 1
up to 2
got 2
up to 3
got 3
over
undefined
> 
> var spr = [0, ...four()]
up to 1
up to 2
up to 3
over
undefined
> 

In other words, we can't use [...generator()] as it eagerly realises the generator. We could modify the signature of descendants to say it returns an iterable, not an array; but we will have to handle it downstream (in select).

let result = [location]
for (let pred of predicates) {
if (R.is(String)(pred)) {
result = result.map(loc => elementChild(pred)(loc)).filter(l => !!l)
Copy link
Member

Choose a reason for hiding this comment

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

Our idiomatic use of a keyPath so far has been to refer to a single child or multiple children under a key. E.g. uiFor('bla') will render a single child under bla or all children found there if it is an array.

We need to keep the same. Does elementChild do that? If it does, the name is misleading.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I know you are trying to wrap your head around currying but

elementChild(pred)(loc)

Is just damn ugly. elementChild(pred, loc) please.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have any issue wrapping my head with curring. Ugly is your conversing with me, and also commenting on outdated code (elementChild has been childAt for a while).

The usage is changed as you suggest. Also, I will revisit childAt one more time, as it is the only thing that doesn't make select generic.

Regarding this:

Our idiomatic use of a keyPath so far has been to refer to a single child or multiple children under a key. E.g. uiFor('bla') will render a single child under bla or all children found there if it is an array.

No, we don't implement childAt like so at the moment.

packages/core/src/zip/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/select.js Outdated Show resolved Hide resolved
packages/core/src/zip/select.js Show resolved Hide resolved
packages/core/src/zip/select.js Outdated Show resolved Hide resolved
@andon
Copy link
Member Author

andon commented Jan 7, 2019

🤔 ... about the following requirement:

  • a 'string' which is interpreted as descending into a property. The property should have an element (or child elements)
  • an array of strings which is interpreted as the predicate ofKind

... even though it is implemented at the moment, it might be an unnecessary complication:

  • The first requirement, descending into a property is not generic. We have two options there, to use the current childAt motion (an implementation that is not clear at the moment), or to use the childrenAt selector. Both of them are "skele" specific. select on the other hand, is generic.
  • The second requirement, to use an array of strings, might not be better than just writing ofKind predicate. This is especially noticeable when both string (descending) and string-array (of-kind) are used together. You really need to think/know what the syntax means.

Maybe we drop this requirement?
@ognen What do you think?

@ognen
Copy link
Member

ognen commented Jan 9, 2019 via email

@andon
Copy link
Member Author

andon commented Jan 10, 2019

@ognen okey, I left it be as it is.

I also left select to have the "skele"-related behavior (ofKind and childrenAt logic). So it is "generic" when you only use functions, and when you deliberately use string or string-array, you use it with "skele"-specifics. I think this should be okey.

I deleted the motion childAt and we use childrenAt now. This should clear up usage we discussed. Also, all selector functions are now iterables.

Will merge it, unless there are some more comments.

@andon andon merged commit 6abab00 into master Jan 12, 2019
@andon andon deleted the feat/zip-select branch January 12, 2019 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants