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

handle ::shorthand/keywords #1

Merged
merged 3 commits into from
Dec 5, 2016
Merged

Conversation

bendlas
Copy link
Contributor

@bendlas bendlas commented Dec 3, 2016

right now, cljsee throws when encountering ::alias/keywords. This patch fixes it.

@aengelberg
Copy link
Owner

Thank you for the pull request. Before merging, I'd prefer to have a new entry in the unit tests that confirms that the parser not only makes it through the string without throwing an error, but also can still white out source code correctly with a double-colon keyword.

(defn disabled-read-keyword
"Read ::foo/keywords without context"
[rdr _ opts pending-forms]
(let [form (#'rdr/read* rdr true nil opts pending-forms)]
Copy link
Owner

Choose a reason for hiding this comment

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

This is a more general, possibly unpredictable attempt at reading keywords. For example, if I'm interpreting the implications of this implementation correctly, ::::::[::#{}] is now a valid keyword. To be on the safer side, could you please rewrite this as a similar implementation to the real read-keyword, that simply eliminates the "second :" logic?

@bendlas
Copy link
Contributor Author

bendlas commented Dec 4, 2016

certainly, here are the tests

@bendlas
Copy link
Contributor Author

bendlas commented Dec 4, 2016

I've implemented slightly more sophisticated parsing of keywords. Not because I think we should reject invalid clojure, but because of a fix for :2number-keywords.

@aengelberg
Copy link
Owner

Looks good. Thanks again. When I get the chance sometime today, I will merge / deploy this.

@aengelberg aengelberg merged commit ce753f4 into aengelberg:master Dec 5, 2016
@aengelberg
Copy link
Owner

Deployed 0.1.1-SNAPSHOT

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.

2 participants