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

Create a comma node and add parser support #44

Merged
merged 8 commits into from Mar 29, 2016

Conversation

Projects
None yet
2 participants
@arrdem
Contributor

arrdem commented Mar 27, 2016

This is more of a RFC than a finished PR, as this badly breaks the test suite and requires a change to the programming interface.

While "," is lexical whitespace in Clojure, it doesn't make sense from the perspective of indentation or rewriting tools to treat it as such. In the case of trailing commas it works ... okay. Because "," is treated as whitespace tools like cljfmt which remove trailing "whitespace" will tend to rewrite comma paired maps. From the indenter's perspective there really isn't a good logical way around this, as preserving trailing commas requires introspecting and rewriting trailing whitespace to minimize it, rather than simply understanding that #",+" is significant whitespace which should not simply be discarded.

I consider the two following cases to be "correct" understandings of "," usage.

Ex 1.

{:foo :bar,
 :baz :qux}

Ex 2.

(cond
  (a-pred? ...)
  ,,(a-fn ...))

Neither of which is enabled by the present strict understanding of "," as whitespace.

This PR has the following impact:

  • Creates a node representing one or more commas
  • Refactors whitespace-node to strictly represent #"\s+"
  • Refactors whitespace-nodes to parse comma nodes as well
  • Simplifies some parser logic by reusing predicates from the reader
  • Extends the whitespace? predicate so that comma nodes are treated as whitespace
  • Exposes whitespace-nodes as part of the API
  • Refactors existing code to make use of whitespace-nodes instead of whitespace-node where appropriate

Because of the last refactor, this is technically a breaking change. I personally don't think it makes sense to overload the existing whitespace-node constructor to potentially generate several nodes rather than one, but the consequence is that consumers are now expected to make use of whitespace-nodes in order to achieve the same semantics 😞

However this enables tooling to opt into treating "," specially using the comma? predicate while preserving existing behavior derived from the whitespace? predicate.

@xsc xsc self-assigned this Mar 28, 2016

@xsc

This comment has been minimized.

Show comment
Hide comment
@xsc

xsc Mar 28, 2016

Owner

@arrdem The proposal seems very reasonable to me. Also, since the next release will include a bump to the minor version anyways, I think we can include this minimally backwards-incompatible change.

I set up a branch based on yours and made the tests pass:

https://github.com/xsc/rewrite-clj/tree/arrdem/comma-nodes

Feel free to pull. :)

Owner

xsc commented Mar 28, 2016

@arrdem The proposal seems very reasonable to me. Also, since the next release will include a bump to the minor version anyways, I think we can include this minimally backwards-incompatible change.

I set up a branch based on yours and made the tests pass:

https://github.com/xsc/rewrite-clj/tree/arrdem/comma-nodes

Feel free to pull. :)

@arrdem

This comment has been minimized.

Show comment
Hide comment
@arrdem

arrdem Mar 28, 2016

Contributor

Pulled your changes and pushed a few more.

As to semver... you're still on 0.n.m so I won't brook strong objection, however as a matter of principle I take great umbrage with this version pattern. 1.0.0 doesn't actually mean you're stable :P

Contributor

arrdem commented Mar 28, 2016

Pulled your changes and pushed a few more.

As to semver... you're still on 0.n.m so I won't brook strong objection, however as a matter of principle I take great umbrage with this version pattern. 1.0.0 doesn't actually mean you're stable :P

@arrdem arrdem referenced this pull request Mar 28, 2016

Open

Discards lexical , #67

(reader/read-while reader reader/linebreak?))
(node/whitespace-node
(reader/read-while reader reader/space?))))
(let [c (reader/peek reader)]

This comment has been minimized.

@arrdem

arrdem Mar 29, 2016

Contributor

Okay good you fixed this.

@arrdem

arrdem Mar 29, 2016

Contributor

Okay good you fixed this.

@xsc xsc merged commit 4e9efd9 into xsc:master Mar 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xsc

This comment has been minimized.

Show comment
Hide comment
@xsc

xsc Mar 29, 2016

Owner

Once #45 is merged, I'll release.

Owner

xsc commented Mar 29, 2016

Once #45 is merged, I'll release.

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