Skip to content

Conversation

@djblue
Copy link
Contributor

@djblue djblue commented Nov 2, 2024

This is a first draft at getting tagged literals working. I don't have much experience with python, so I may have made some amateur mistakes.

Copy link
Member

@chrisrink10 chrisrink10 left a comment

Choose a reason for hiding this comment

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

Looks cool! I was not aware of this feature, so thanks for submitting the PR. Looks pretty good to me but I did have a few minor comments.

Does this need to actually be integrated with the reader at some point in time? Right now the reader handles tagged literal forms (which requires each subsequent builtin form to implement logic in the reader, analyzer, and generator 😵 ). I filed #604 years ago and maybe emitting these straight from the reader could simplify the logic of each of those components considerably but I'm not sure how these are used in Clojure.

@chrisrink10
Copy link
Member

Also I generally like to track PRs with issues as well so I went ahead and filed #1104 which you can add this as the fix PR.

@djblue
Copy link
Contributor Author

djblue commented Nov 8, 2024

@chrisrink10 I updated the PR with your suggestions, thanks for the quick feedback, sorry I took a bit to get the changes up.

I don't think tagged-literals need to be wired directly into the reader. I opt-in to using them with something like:

(require '[clojure.edn :as edn])
(edn/read-string {:default tagged-literal} "#js [1 2 3]")

Although, I did notice that my reader conditionals for cljs do cause issues in basilisp:

#?(:cljs #js [] :default [])

Copy link
Member

@chrisrink10 chrisrink10 left a comment

Choose a reason for hiding this comment

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

Just a couple of very minor tweaks and this looks good to me. Thanks again for the PR 🙏

@chrisrink10
Copy link
Member

chrisrink10 commented Nov 8, 2024

I don't think tagged-literals need to be wired directly into the reader. I opt-in to using them with something like:

(require '[clojure.edn :as edn])
(edn/read-string {:default tagged-literal} "#js [1 2 3]")

Although, I did notice that my reader conditionals for cljs do cause issues in basilisp:

#?(:cljs #js [] :default [])

Ah, maybe in another ticket we can change the reader to emit tagged-literals whenever the tag isn't recognized? I'll probably need to take a look at how it works in Clojure a little bit more closely.

chrisrink10
chrisrink10 previously approved these changes Nov 8, 2024
@chrisrink10
Copy link
Member

Oh shoot sorry. Could I get you to add a changelog line referencing #1104?

chrisrink10
chrisrink10 previously approved these changes Nov 8, 2024
@chrisrink10
Copy link
Member

Filed #1118 for the issue you mentioned

@chrisrink10
Copy link
Member

Looks like you have some sort of code formatting issue. Not sure about your local dev setup but if you have a Python virtualenv configured you could make format and push.

@chrisrink10 chrisrink10 merged commit 1701536 into basilisp-lang:main Nov 8, 2024
12 checks passed
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