-
Notifications
You must be signed in to change notification settings - Fork 631
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(functions): adding a functions module with pipe() #6143
base: main
Are you sure you want to change the base?
Conversation
Are I personally think inline arrow functions express the operation like const myNewFunc = set_arguments(myFunc, 1, undefined, 3);
// vs
const myNewFunc = (x: number) => myFunc(1, x, 3); |
Both
|
I don't understand this. Inline arrow function example should work with any parameters available in that scope. I think that has the same capability as what BTW do you suggest
Can you elaborate on more specific examples where const func = pipe(Math.abs, Math.sqrt, Math.floor);
//
const func = (num: number) => Math.floor(Math.sqrt(Math.abs(num))) |
I see Yes, of course, you can create an arrow function to inject parameters: const fnWithFirstDeps = (dataArg: Parameters<typeof fn>[2]) => fn(dependency1, dependency2, dataArg) I think it's a common and rather generic need, so having a utility that obviates the need to create a local function for it and improves readability is handy, while also encouraging a good coding practice (dependency injection). Sometimes different parts of the code inject different dependencies, having a utility encourages doing the injection locally and not creating another function or re-using the above function which entangles concerns. const fnWithFirstDeps = setArguments(fn, dependency1, dependency2,)
const extracted = {
bedrooms: pipe(
selectOne.attributes,
parseMatchingText("bedrooms"),
removeLabel,
parseNumber,
),
bathrooms: pipe(
selectOne.attributes,
h.parseMatchingText("bathrooms"),
removeLabel,
parseNumber,
),
} |
I'm also not a fan of these typings. |
(BTW while I'm personally not in favor of this, we are open for adding this package if there's enough community support to this idea.) |
Typing for a number of arguments is very common and gives a good experience. As far as I can see, it's a theoretical, not a practical issue. |
This PR and #4386, which has been open since February, have had near-zero community support. Perhaps, it'd be best to have this done as a 3rd party package and see if it evolves enough to provide a stronger argument for its addition sometime in the future. |
Honestly |
@guy-borderless Can you limit this PR to |
Of course! On it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6143 +/- ##
==========================================
+ Coverage 96.29% 96.32% +0.03%
==========================================
Files 493 544 +51
Lines 39530 41610 +2080
Branches 5833 6313 +480
==========================================
+ Hits 38065 40082 +2017
- Misses 1423 1485 +62
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. |
c7df99b
to
3a08d69
Compare
Per feedback, PR was limited to pipe() and tests were added. |
You also got to get all the tests to pass. Looks like the title isn't named right and you're missing the copyright notice in some of the files. |
The PR naming issue seems to be because the functions module is new and so a scope doesn't exist for it. Added the copyright notice, thanks for mentioning it (didn't seem to cause a test to fail) |
bd18d57
to
9fc723a
Compare
"imports": { | ||
"@deno/doc": "jsr:@deno/doc@0.137", | ||
"@deno/graph": "jsr:@deno/graph@^0.74", | ||
"@std/archive": "jsr:@std/archive@^0.225.0", | ||
"@std/assert": "jsr:@std/assert@^1.0.2", | ||
"@std/async": "jsr:@std/async@^1.0.3", | ||
"@std/bytes": "jsr:@std/bytes@^1.0.2-rc.3", | ||
"@std/cli": "jsr:@std/cli@^1.0.3", | ||
"@std/collections": "jsr:@std/collections@^1.0.5", | ||
"@std/crypto": "jsr:@std/crypto@^1.0.2-rc.1", | ||
"@std/csv": "jsr:@std/csv@^1.0.1", | ||
"@std/data-structures": "jsr:@std/data-structures@^1.0.1", | ||
"@std/datetime": "jsr:@std/datetime@^0.224.5", | ||
"@std/dotenv": "jsr:@std/dotenv@^0.225.0", | ||
"@std/encoding": "jsr:@std/encoding@^1.0.1", | ||
"@std/expect": "jsr:@std/expect@^1.0.0", | ||
"@std/fmt": "jsr:@std/fmt@^1.0.0", | ||
"@std/front-matter": "jsr:@std/front-matter@^1.0.1", | ||
"@std/fs": "jsr:@std/fs@^1.0.1", | ||
"@std/html": "jsr:@std/html@^1.0.1", | ||
"@std/http": "jsr:@std/http@^1.0.2", | ||
"@std/ini": "jsr:@std/ini@^1.0.0-rc.3", | ||
"@std/internal": "jsr:@std/internal@^1.0.1", | ||
"@std/io": "jsr:@std/io@^0.224.4", | ||
"@std/json": "jsr:@std/json@^1.0.0", | ||
"@std/jsonc": "jsr:@std/jsonc@^1.0.0", | ||
"@std/log": "jsr:@std/log@^0.224.5", | ||
"@std/media-types": "jsr:@std/media-types@^1.0.2", | ||
"@std/msgpack": "jsr:@std/msgpack@^1.0.0", | ||
"@std/net": "jsr:@std/net@^1.0.0", | ||
"@std/path": "jsr:@std/path@^1.0.2", | ||
"@std/regexp": "jsr:@std/regexp@^1.0.0", | ||
"@std/semver": "jsr:@std/semver@^1.0.1", | ||
"@std/streams": "jsr:@std/streams@^1.0.1", | ||
"@std/testing": "jsr:@std/testing@^1.0.0", | ||
"@std/text": "jsr:@std/text@^1.0.2", | ||
"@std/toml": "jsr:@std/toml@^1.0.0", | ||
"@std/ulid": "jsr:@std/ulid@^1.0.0", | ||
"@std/url": "jsr:@std/url@^0.225.0", | ||
"@std/uuid": "jsr:@std/uuid@^1.0.0", | ||
"@std/webgpu": "jsr:@std/webgpu@^0.224.5", | ||
"@std/yaml": "jsr:@std/yaml@^1.0.2", | ||
"automation/": "https://raw.githubusercontent.com/denoland/automation/0.10.0/", | ||
"graphviz": "npm:node-graphviz@^0.1.1", | ||
"npm:/typescript": "npm:typescript@5.4.4" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the importMap
here for imports
? This will probably need to be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "@std/functions": "jsr:@std/functions@^0.1.0"
will also probably need to be added to the importMap
for the later lint error to fix itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert these changes as @BlackAsLight points. import_map.json
is shared between deno.json
and browser-compat.tsconfig.json
. That is why we don't write these mappings here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the
"importMap": "./import_map.json",
removed by mistake.
Is that all?
functions/pipe.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These exports are also going to need JSDoc comments to satisfy the linter. You can do deno task ok
locally to check that it satisfies the requirements.
I believe a change needs to happen in |
functions/mod.ts
Outdated
/** | ||
* Pure functions for common tasks around collection types like arrays and | ||
* objects. | ||
* | ||
* Inspired by | ||
* {@link https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/ | Kotlin's Collections} | ||
* package and {@link https://lodash.com/ | Lodash}. | ||
* | ||
* ```ts | ||
* import { intersect, sample, pick } from "@std/collections"; | ||
* import { assertEquals, assertArrayIncludes } from "@std/assert"; | ||
* | ||
* const lisaInterests = ["Cooking", "Music", "Hiking"]; | ||
* const kimInterests = ["Music", "Tennis", "Cooking"]; | ||
* | ||
* assertEquals(intersect(lisaInterests, kimInterests), ["Cooking", "Music"]); | ||
* | ||
* assertArrayIncludes(lisaInterests, [sample(lisaInterests)]); | ||
* | ||
* const cat = { name: "Lulu", age: 3, breed: "Ragdoll" }; | ||
* | ||
* assertEquals(pick(cat, ["name", "breed"]), { name: "Lulu", breed: "Ragdoll"}); | ||
* ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't describe this module
should I add the functions module to import_map.json here? |
Yes |
Hi,
I'd like to suggest adding an std module for utilities related to functions.
related: #4386
There are common manipulations to functions implemented in numerous npm packages and toolbelts, and using them where appropriate greatly improves code quality. It would be handy to have these in deno_std.
I want to suggest for starters the following two:
These two scenarios have some typescript knowledge behind them and it makes sense to reach for a proper utility.
Both of these have many different implementations in terms of code and typings. In my opinion, the implementation most in line with the pragmatic spirit of the std should avoid functional programming jargon for simplicity and accessibility to beginners.
For discussion in this PR I have included two implementations (without tests) that I think would suit the deno_std
pipe has good typescript performance and small implementation. I'm also using it in several projects.
set_arguments has a very non-academic intuitive signature that I think suites nicely with the deno_std. This signature does not expect a data-last coding convention, so it doesn't leak to the rest of the code. I have also provided a more classic functional programming implementation of this called curry to contrast.
I can significantly simplify the typing definition of
set_arguments
if you'd like me to go ahead with preparing this PR.Since functions are an important primitive in JS, I'm sure more utilities will be added.