Make methods return Unit #53
Conversation
|
Thanks! What do you think we should/can do about the |
|
The OO part of me likes Context2D first but for PS functions, I want the arguments most likely to be partially-applied to come first. Agree it should be consistent to reduce friction. Otherwise, I don't have enough experience using the library to make a good recommendation. |
|
I think we might want to partially apply either the arguments or the context, so I'm really not sure which is best :) but you're right that we should be consistent. I think it's probably more likely that we'll want to partially apply the context, so let's go with that. Would you like to make that change? Or if you don't have time, I can take a look after this gets merged. Thanks! |
|
happy to do it |
|
Starting the re-ordering. Maybe I should also similarly re-order the Canvas operations. Thoughts? |
|
Additional changes:
|
| ``` | ||
|
|
||
| Set the current font. | ||
|
|
||
| #### `fillText` | ||
|
|
||
| ``` purescript | ||
| fillText :: forall eff. Context2D -> String -> Number -> Number -> Eff (canvas :: CANVAS | eff) Context2D | ||
| fillText :: forall eff. Context2D -> String -> Number -> Number -> Eff (canvas :: CANVAS | eff) Unit |
There was a problem hiding this comment.
I mean, is there a reason why Context2D isn't last here like it is in the other methods?
There was a problem hiding this comment.
context2d should be first. that's what we discussed before. it's just that the docs aren't updated for the others. i'll commit a document update
There was a problem hiding this comment.
that's done. i hadn't realized generating documents is not part of the build
src/Graphics/Canvas.js
Outdated
| return function() { | ||
| canvas.width = width; | ||
| return canvas; | ||
| return {}; |
There was a problem hiding this comment.
For reference, you don't need these, since values of type Unit can't be inspected, so you can return any value and the user won't be able to observe it.
There could be a small speedup to be had by not constructing these values.
There was a problem hiding this comment.
sure. that's easy to change. just pushed an update
src/Graphics/Canvas.js
Outdated
| @@ -1,6 +1,8 @@ | |||
| /* global exports */ | |||
| "use strict"; | |||
|
|
|||
| const unit = {}; | |||
There was a problem hiding this comment.
My point was that you don't need to return anything in particular (or at all). Just omitting the return statement will be fine.
There was a problem hiding this comment.
Omitted. I would normally favor the explicit returns since they document intent.
psc-package.json
Outdated
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "canvas", | |||
| "source": "https://github.com/purescript/package-sets.git", | |||
| "set": "psc-0.10.1", | |||
| "set": "psc-0.11.5", | |||
There was a problem hiding this comment.
You can just delete this file actually, it's not being used.
|
@paf31 this looks good to me; do you have anything else to add or would you say this is good to merge? |
|
No, this looks great, I think we should release it. Thanks for your patience @aztecrex :) |
|
Great. I'd be happy to do so, although I don't have push access to this repo right now. |
|
Can you please try now? |
|
Seems to work, thanks! I'll make a release when I'm next at my PC (on mobile right now). |
|
Actually, I was thinking I might try to address #22 first, if that sounds sensible? |
|
👍 Yep sounds good to me, thanks! |
No description provided.