Skip to content
This repository has been archived by the owner on Oct 4, 2020. It is now read-only.

Add Selection API #81

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

paulyoung
Copy link

This adds the Selection API as documented on MDN. Non-standard features such as modify are not included.

Support for this API seems very good according to Can I use.

I would appreciate as much feedback as possible on this, particularly on the introduction of the SELECTION effect, and around the effects used in general.

Some feedback on the approach used for type would also be great.

I think it also makes sense to add more on the Range data type. I'd like to do this in accordance with the documentation on MDN but I'm not sure what the appropriate effect should be.

Thanks in advance.

@garyb
Copy link
Member

garyb commented Mar 16, 2017

I think this looks great. Re: type_, we can probably drop the Partial via unsafePartial - I think I've done that elsewhere whenever we implement the enumeration of possible string values according to the spec.

@paulyoung
Copy link
Author

I fixed the conflicts with master locally but now I'm getting this:

[1/1 ScopeConflict] bower_components/purescript-eff/src/Control/Monad/Eff.purs:36:1

  36  foreign import data Eff :: # Effect -> Type -> Type
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  Conflicting definitions are in scope for kind Effect from the following modules:

    Control.Monad.Eff
    Prim

@paulyoung
Copy link
Author

Travis is OK with this so not sure what's happening locally.

@garyb
Copy link
Member

garyb commented Apr 19, 2017

That's the error you get if you try and compile 0.11 libraries with 0.10 - maybe old compiler in node_module getting picked up if you installed the npm dependencies locally? :)

@paulyoung
Copy link
Author

@garyb I just fixed the latest conflicts. Please let me know if you think anything else needs to change here.

@paulyoung
Copy link
Author

@garyb would you mind taking another look at this please?

-> Node
-> Int
-> Selection
-> Eff (selection :: SELECTION | eff) Unit
Copy link
Member

Choose a reason for hiding this comment

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

I think all of these latter functions should perhaps have the Selection argument first for "normal" PS argument ordering?

Copy link
Member

Choose a reason for hiding this comment

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

Wait, disregard that, I think it probably is the right way around now.

I'll let you comment on that either way and then will merge :)

Copy link
Author

Choose a reason for hiding this comment

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

Now you've got me wondering!

Copy link
Member

Choose a reason for hiding this comment

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

I think it's tricky with this one, as it's usually "the thing that is being operated on goes last" - but due to the nature of this API, the selection is acting on a thing - that's what tripped me up.

I'm fine with it either way, just let me know what you want to do, now I've sown the seeds of doubt 😆

@garyb
Copy link
Member

garyb commented Sep 22, 2017

I was a little apprehensive about adding this, since it is still experimental and the spec is just a working draft... but at least there is a spec! And it's decently supported as you point out.

Sorry I ignored it for ages!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants