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

Add functional versions of threading macros #1489

Closed
wants to merge 2 commits into from

Conversation

na-sa-do
Copy link

The proper solution to #1477. Or at least the most proper solution practicable. (Life got in the way of making this PR sooner.)

Copy link
Member

@pepe pepe left a comment

Choose a reason for hiding this comment

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

I do not see this as a valuable addition to the standard library, but it is subjective and will not be decided here.

But if this is considered, I would like to see all the variants, not only the -> but also the ->> and so on. I am also missing the tests.

@na-sa-do
Copy link
Author

I don't think the other variants make sense. Since there's no way to provide additional arguments, fn-> only works when all its arguments are unary functions, so a hypothetical fn->> would behave identically to fn->. Ditto for the short-circuit and as versions.

I did forget testing, but I just grepped and it looks like the functionality of the macros isn't tested anywhere either, as far as I can tell. There are tests that confirm the sourcemaps work right, and there are lots of tests that use the threading macros, but none that seem to be focused on them. So I'm not sure if I should rectify that as well, or (if it matters) where they should go.

@bakpakin
Copy link
Member

fn-> seems pretty similar to comp; function composition but in reverse.

(defn f1 [x] (+ 2 x))
(defn f2 [x] (* x x))
(defn f3 [x] (math/exp x))

(fn-> 1 f1 f2 f3) # -> 8103.08392757538
((comp f3 f2 f1) 1) # -> 8103.08392757538

The short circuit pipeline seems like it could be useful, although it would be more efficient with just a plain while loop instead of prompt + return.

@sogaiu
Copy link
Contributor

sogaiu commented Aug 23, 2024

I'm not sure there was agreement that #1477 counted as a problem among existing users of Janet, but I don't oppose the idea of adding potentially useful functions.

If this is to be added, I think it makes sense to add some tests for it. It's not just about testing -- tests can also function as a place to demonstrate how things work. As to where to put tests, if no other good location can be thought of, I think the suite-boot.janet file could work. If you wanted to add tests for the existing constructs, I think that could be a plus, but I don't think it's necessary.

If there are tests added, may be it could be sensible to try to demonstrate a bit of why this construct is useful (say over the existing ones).

Also, not fond of reusing x like:

(var x x)

@pyrmont
Copy link
Contributor

pyrmont commented Aug 23, 2024

I do not see this as a valuable addition to the standard library, but it is subjective and will not be decided here.

I think it will be decided here, @pepe. If this PR is accepted, it's in the Core API.

I apologise in advance for the negativity but I am concerned that this addition would contribute to bloat in the Core API (something that's always a struggle to fight against for everyone including me).

In addition to that more general point, I make the following (somewhat pedantic) observations:

  1. The existence of these functions would not have solved the original problem, which, as I understand it, was an incorrect understanding of how macros work. Even if the argument is that the way the macros work is not helpful for all use cases and so functional alternatives are necessary, how would a user know these exist? The functions are named such that they will be sorted far away from the macro forms in the Core API documentation. Nobody looking at the documentation for -> or -?> will easily see that there are functional equivalents (contrast with, for example, juxt and juxt*).

  2. Speaking of which, the naming is inconsistent with the way that macro/functional version of other forms are named (i.e. doc, import and juxt). If it were, it would be named ->* and -?>*.

  3. It's not obvious to me that there is widespread confusion about how threading macros work that justify putting this in the Core API. Users who want this functionality should be encouraged to define functions that work in the way this PR does. Janet and JPM make it easy for functions to be packaged up into a module, made generally available through a repository-hosting service like GitHub and then imported into projects as and when necessary.

@na-sa-do
Copy link
Author

@sogaiu: Do people read tests as a kind of documentation? They don't seem like the most approachable way to do it, given that we have janetdocs.com.

Also, could you be more specific about the x thing? Is there a better way to make a function parameter name mutable?


@pyrmont: I didn't know there was a convention for that. I'll change the names. That said, putting these two functions in a third-party package would make them even less discoverable, and also risk NPM syndrome -- to quote the JPM docs:

As a matter of style, it is also recommended to group small libraries together into "bundles" that are updated, tested, and deployed together.

I think part of the point of a standard library is, or at least should be, to be such a "bundle" for tiny utilities like these. There is of course also the option to make an underscore/lodash-style generic utility library, but I think those are also an anti-pattern -- unavoidable in JS's case, since it's a standardized language and so develops much slower than the ecosystem can, but not necessary for Janet. Not to mention I don't have any more small utilities on hand to collect into such a library.

@bakpakin
Copy link
Member

Given the conversation here, I will not merge the fn-> as it is a slightly less general version of comp. As for the other function, I think that it could be somewhat useful but probably not in core. That are a number of functions in core that I would remove if not for backwards compatibilities sake (juxt and the walk-* functions are my least favorite). Every function we add to the core module increase binary size, initial memory usage, and makes the language slightly harder to keep in your head.

I do like the functionality of 'fn-?>' (not the name) although probably it would probably be best in a utility bundle such as spork.

@bakpakin bakpakin closed this Aug 24, 2024
@na-sa-do na-sa-do deleted the fn-arrow branch August 24, 2024 03:02
@sogaiu
Copy link
Contributor

sogaiu commented Aug 24, 2024

@na-sa-do Regarding whether people read tests as a kind of documentation...

In my experience some people do this as a supplementary activity, but sometimes there is no other documentation. Also, prose can be helpful, but it is expensive to verify and keep up-to-date compared to tests. It's not a one or the other sort of thing though, I think it's often better to have both (though I'm not a fan of having examples within docs if the examples cannot be kept working via automation).

In the context of this PR, my impression was that a number of us remain unconvinced of how much value the proposed two functions brought to the table. To some extent, we act as representatives of the community [1] and if we are not convinced, it is up to the proposer to make a good case [2]. Specific usage examples might have helped, and one way to exhibit this is via tests (which if accepted, would be more likely to be kept up-to-date and also provided the usual benefits of tests (e.g. detecting breakage)). Such tests would then be available for future readers who might also have wondered what kind of value the two functions might have been bringing.

I have found that concrete examples can be illustrative and reduce ambiguities and/or misunderstanding. It's important though that they are and remain valid.

As an example of this, consider this directory. It is populated with usage examples of PEGs for Janet. Every PEG special in Janet is covered here and one can see how to varying degrees the PEG system can be used.

The examples there double as tests -- I run these tests to check my Janet implementation of Janet's PEG system, but I also use the tests to test the C implementation of Janet's PEG system from time to time.

Possibly you might find this post by @ianthehenry to be of interest. He specifically has a section titled "Good tests make good documentation".

The above were a couple of examples from Janet-using folks, but this is not the first place I noticed this kind of thing [3].


[1] Everyone who has responded to this PR has been using Janet over the course of some years now.

[2] Ultimately it's up to bakpakin, but as can be seen in his closing comment, he does factor in feedback from others.

[3] One example from Rust can be seen in tree-sitter's repository. For a long time, which regular expression features were supported in tree-sitter's flavor of regular expression for its DSL for expressing grammars was unclear and one way to gain insight into this was via the tests here.

Clojure has some of this sort of thing too. Here is a bit of it from Clojure's source. That specific pattern has spread more in the Clojure community and has the name "Rich Comment Block". Here is a bit on it if interested.

@pepe
Copy link
Member

pepe commented Aug 24, 2024

@pyrmont I meant by me with "here", sorry for the confusion by my bad English.

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

Successfully merging this pull request may close these issues.

5 participants