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

do (doseq etc.) body should be newline seperated #3

Closed
pesterhazy opened this issue Dec 6, 2016 · 9 comments
Closed

do (doseq etc.) body should be newline seperated #3

pesterhazy opened this issue Dec 6, 2016 · 9 comments
Assignees

Comments

@pesterhazy
Copy link
Contributor

boot.user=> (->> (zprint.core/zprint-str "(doseq [x xs]\n  (foo x)\n  (bar x))" {:parse-string-all? true, :parse {:interpose "\n\n"}}) println)
(doseq [x xs] (foo x) (bar x))

Based on clojure code I've read, I would expect each form in the body to be separated by a newline for do and do-likes (doseq, dotimes, dosync).

I think the same is true for a lot of other macros as well, e.g. with-open, future

@kkinnear kkinnear self-assigned this Dec 7, 2016
@kkinnear
Copy link
Owner

kkinnear commented Dec 7, 2016

Thanks for bringing this up!

I'm on the fence for this one. Personally, I like to see as much as I can in an editor window, so I'm not wild about having my short doseq functions taking up three lines instead of one line. However, I certainly think that it should be possible to configure zprint to do that if you want to do that. I assume that what you want here is:

(doseq [x xs]
  (foo x)
  (bar x))

Currently, you can not really get zprint to do that, unfortunately:

"(doseq [x xs] (foo x) (bar x))"
zprint.zprint=> (czprint ds {:parse-string? true})
(doseq [x xs] (foo x) (bar x))
nil
zprint.zprint=> (czprint ds {:parse-string? true :fn-map {"doseq" :force-nl-body}})
(doseq [x xs]
       (foo x)
       (bar x))
nil
zprint.zprint=> (czprint ds {:parse-string? true :fn-map {"doseq" :arg1-force-nl}})
(doseq [x xs]
  (foo x)
  (bar x))
nil

The last one looks like it worked, but it didn't really, as the binding form won't be handled as a vector of pairs. What we really need is a :binding-force-nl function type. So that is something I'll have to build, though it won't be hard. I will do that.

The remaining question is -- should all of the binding functions be converted to :binding-force-nl once I get it finished? These are the functions:

   "let" :binding
   "binding" :binding,
   "with-redefs" :binding,
   "with-open" :binding,
   "with-local-vars" :binding,
   "with-bindings" :arg1,
   "with-bindings*" :arg1,
   "when-some" :binding,
   "when-let" :binding,
   "when-first" :binding,
   "loop" :binding,
   "if-some" :binding,
   "if-let" :binding,
   "for" :binding,
   "dotimes" :binding,
   "doseq" :binding

Certainly I've thought a lot about forcing let to have newlines between the body forms. I also have zero idea why I set with-bindings and with-bindings* to be :arg1 and not :binding. I'll have to look into that!

I could be convinced to make :binding-force-nl the default (as opposed to maybe a style). Maybe you could send me some pointers to code on github (perhaps in clojure.core, though not necessarily) that you think looks like good code that does what you want? Probably we should have the default be one way, and have a style that does it the other way.

Of course, you can only configure one style in an options map, say in .zprintrc, so if I keep adding styles I might need to do something different about that. Maybe a vector of styles or something. But that is an enhancement for another day.

@kkinnear
Copy link
Owner

kkinnear commented Dec 9, 2016

Interestingly, when looking at a comment on the other issue you raised, I realized that you can, indeed, configure this:

(doseq [x xs]
  (foo x)
  (bar x))

You would add :binding to the set which is the value of :fn-force-nl. Now, there is a bug, so it isn't trivial to add to that set -- you need to reproduce the whole set. But if you do that, you can get what you want now. At least for all :binding fns. Which may be what you want. But it isn't very fine grained. Thus:

zprint.zprint=> ffn
#{:force-nl :noarg1 :noarg1-body :force-nl-body :arg1-force-nl}
zprint.zprint=> ds
"(doseq [x xs] (foo x) (bar x))"
zprint.zprint=> (czprint ds {:parse-string? true :fn-force-nl (into #{} (concat ffn #{:binding}))})
(doseq [x xs]
  (foo x)
  (bar x))

My question for you: Do you need to configure this fn by fn (i.e., do I need to build a specific :fn-type for this), or are you ok with just having all :binding fns be multi-line?

@pesterhazy
Copy link
Contributor Author

Thanks for your response! You're right, I prefer

;; newline style
(doseq [x xs]
  (foo x)
  (bar x))

to

;; compressed style
(doseq [x xs]
  (foo x)
  (bar x))

I'll address the issue of why I want doseq (and other forms) to adhere to newline style first.

I think the core of the issue is that typically do is used to indicate imperative-style code. The value of a do block is the value of its last form, so the other forms must be evaluated for side-effects. An example from clojure

(do
   (write-white-space this)
   (.col_write this x))

As a side-note, this is also common in clojure.core:

(do (.appendReplacement m buffer (Matcher/quoteReplacement (f (re-groups m))))
    (recur (.find m)))

but that's a different discussion I think.

As I understand it, people write dos in multiple lines as this reflects the sequential nature of imperative code.

doseq is explicitly imperative (the docstring mentions that the body is executed "presumably for side-effects"). But many other macros (when, time, let) support "implicit do", i.e. they take a body containing multiple forms. For example, when takes [test & body] and wraps the body in a do. The bodies of let, fn and defn are do-likes as well.

@pesterhazy
Copy link
Contributor Author

Empirically, looking at the clojure git repository (git grep -A5 -B0 '(doseq'), all doseqs or dotimes with multiple body forms use newline style.

I did find some doseqs with a single body form written in a single line. Personally I prefer newline style even for single-form doseqs and the majority of those in the clojure use that style. But my main concern is with multiple-form doseqs (and do-likes in general). My personal sense is that compressed style for these obscures the imperative structure of the code.

@pesterhazy
Copy link
Contributor Author

I should add that these are all debatable and I may be wrong in my judgments about community conventions. Ideally all of this should be configurable (and making this possible is IMO the most powerful thing about zprint).

Having said that, IMO a library like zprint (like a style guide) should aim to have defaults (or defaults easily accessible) that best reflect community conventions. Ideally we should be able to say, "If in doubt, just let zprint (lein-zprint, boot fmt) handle it", similar to how things work in the golang community.

@pesterhazy
Copy link
Contributor Author

To your final question, I'll have to look more into how zprint configuration works to give a more informed answer.

@kkinnear
Copy link
Owner

Thanks for the discussion! It is pleasure to have someone to discuss this with, actually.

First, I totally agree that:

  1. All of this (i.e., the multi-line nature of do-likes) should be configurable.
  2. zprint, by default, should do a nice job. You shouldn't have to bother to configure zprint unless you want something a particular way. (Which is not exactly what you said, I acknowledge).

I'm completely on board with the idea that by zprint should be easily configurable in order to follow community conventions. That said, there are multiple communitiies.

The community-style-guide has what are to me very strange conventions about the indent depth based on whether or not a function has "body parameters" or "arguments". Personally, I see them pretty much the same way, and I have found limited actual code doing this in practice, though it has been a while since I looked. I did implement this capability in zprint, because I thought it was important to be able to do it the way the style guide says, but I am not keen to make that the default.

I tend to look at clojure.core as a large part of the community, though it is far from internally consistent.

For defaults I tend to pick what I like from the various example "communities". Plus my own idiosyncratic issues, like indented second things in cond and initializers in let.

Enough philosophy, let's talk do-like details.

I found this function in zprint:

(defn map-stop-on-nil
  "A utility function to stop on nil."
  [f out in]
  (when out (let [result (f in)] (when result (conj out result)))))

This must drive you crazy. I realize that an if-let would make it clearer, but
that's not today's point. You would like it to be:

(defn map-stop-on-nil
  "A utility function to stop on nil."
  [f out in]
  (when out 
    (let [result (f in)] 
      (when result 
        (conj out result)))))

and I don't disagree.

I have added, but not pushed to github, a :fn-type of :gt2-force-nl, to try to get some 1.9-alpha clojure.core spec files to look better than they do today without also looking worse. The idea here was to have s/or be :gt2-force-nl, where if there were more than two arguments, the s/or would be force-nl. So, you get this:

(s/or :sym ::local-name 
      :seq ::seq-binding-form 
      :map ::map-binding-form))

[The reason you this the pairs is because of "constant-pairing", just FYI.]

but you also get this:

(s/or :sym ::local-name)

as opposed to this:

(s/or :sym 
      ::local-name)

which is what you would get if it were :force-nl like s/cat is. This is what the spec file does
today with s/or.

What I am thinking now is that instead of a :fn-type of :gt2-force-nl, that there would be several sets like :fn-force-nl, e.g. :gt2-force-nl, :gt3-force-nl. This way if doseq was :binding, and :binding was in :gt2-force-nl, then you would get this:

(doseq [x xs] (dostuff x))

and this

(doseq [x xs] 
  (stuff x)
  (bother x))

If, on the other hand, :binding was in :fn-force-nl, then you would get this:

(doseq [x xs]
  (dostuff x))

(doseq [x xs]
  (stuff x)
  (bother x))

This doesn't let you configure different handling for, say, doseq and let, as both are :binding functions. It is, however, a bit more general as the force-nl configuration is taken from the :fn-type, but is not encoded in it directly. I'm trying to avoid an explosion of :fn-types.

Were I to go this direction, I would have to have some :fn-type to use for s/or and s/and, so I could put them into the :gt2-force-nl set, as they are not otherwise configured as anything today.

How does that sound to you?

@pesterhazy
Copy link
Contributor Author

pesterhazy commented Dec 11, 2016 via email

@kkinnear
Copy link
Owner

This is fixed in 0.2.10.

I ended up creating two new sets, :fn-gt2-force-nl and :fn-gt3-force-nl, which will keep a function type which appears in either set from printing all on one line if it has greater than 2 (gt2) or greater than 3 (gt3) arguments. I have put the :binding in the :fn-gt2-force-nl set, so that a doseq with one thing after the binding will print on one line, and with two things to do after the binding it will print on multiple lines even if it is short. If you always want doseq, or anything with a binding, to print on multiple lines, put :binding in :fn-force-nl instead of :fn-gt2-force-nl, and it will do that.

Thanks for the suggestions!

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

No branches or pull requests

2 participants