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

Configurable escaping #30

Closed
wants to merge 3 commits into from
Closed

Configurable escaping #30

wants to merge 3 commits into from

Conversation

edvorg
Copy link

@edvorg edvorg commented Nov 17, 2015

This change adds escape flag to as-hiccup function to manage its' escaping behaviour.
I also updated a project to new clojurescript version and migrated it to cljc files.
This fixes #25.

@davidsantiago
Copy link
Collaborator

You probably saw my extended comments on #25, about why I don't think this is the right solution. Nonetheless, there's continued interest in this, apparently because some libraries do a poor job of implementing the hiccup syntax, and so perhaps Hickory is in the best position to help.

But this PR has some issues I'd like to see fixed. Updating the Clojure and Clojurescript versions is out of scope for a fix like this. It may well be a good idea, and maybe a good idea for fixing this issue, but that's a separate conversation, and I'd want to hear from more users about whether jumping the version of Clojure required up by 2 is going to provide the value. Similarly, I'd like to hear from @jeluard on the ClojureScript version.

Adding a parameter to as-hiccup is not how to do this. It makes the API incompatible with the existing one, for one thing. Additionally, I don't think it's the correct thing to do even if it was done in a source-compatible way, because it implies that there are two correct formats for Hiccup, when in fact only one way will work when given as input to Hiccup. I think the way to make this change is to add a new protocol, as-hiccup-noescape or as-reagent or something, and keep it clear that its output will not go into Hiccup correctly, nor will Hiccup's go into Reagent's.

@edvorg
Copy link
Author

edvorg commented Nov 17, 2015

OK, thank you for your feedback, I will think about it.

On Tue, Nov 17, 2015, 5:47 AM David Santiago notifications@github.com
wrote:

You probably saw my extended comments on #25
#25, about why I don't
think this is the right solution. Nonetheless, there's continued interest
in this, apparently because some libraries do a poor job of implementing
the hiccup syntax, and so perhaps Hickory is in the best position to help.

But this PR has some issues I'd like to see fixed. Updating the Clojure
and Clojurescript versions is out of scope for a fix like this. It may well
be a good idea, and maybe a good idea for fixing this issue, but that's a
separate conversation, and I'd want to hear from more users about whether
jumping the version of Clojure required up by 2 is going to provide the
value. Similarly, I'd like to hear from @jeluard
https://github.com/jeluard on the ClojureScript version.

Adding a parameter to as-hiccup is not how to do this. It makes the API
incompatible with the existing one, for one thing. Additionally, I don't
think it's the correct thing to do even if it was done in a
source-compatible way, because it implies that there are two correct
formats for Hiccup, when in fact only one way will work when given as input
to Hiccup. I think the way to make this change is to add a new protocol,
as-hiccup-noescape or as-reagent or something, and keep it clear that its
output will not go into Hiccup correctly, nor will Hiccup's go into
Reagent's.


Reply to this email directly or view it on GitHub
#30 (comment).

@edvorg
Copy link
Author

edvorg commented Nov 17, 2015

I will reset changes and reimplement it in way suggested by you.

On Tue, Nov 17, 2015, 11:49 AM Edward Knyshov edvorg@gmail.com wrote:

OK, thank you for your feedback, I will think about it.

On Tue, Nov 17, 2015, 5:47 AM David Santiago notifications@github.com
wrote:

You probably saw my extended comments on #25
#25, about why I don't
think this is the right solution. Nonetheless, there's continued interest
in this, apparently because some libraries do a poor job of implementing
the hiccup syntax, and so perhaps Hickory is in the best position to help.

But this PR has some issues I'd like to see fixed. Updating the Clojure
and Clojurescript versions is out of scope for a fix like this. It may well
be a good idea, and maybe a good idea for fixing this issue, but that's a
separate conversation, and I'd want to hear from more users about whether
jumping the version of Clojure required up by 2 is going to provide the
value. Similarly, I'd like to hear from @jeluard
https://github.com/jeluard on the ClojureScript version.

Adding a parameter to as-hiccup is not how to do this. It makes the API
incompatible with the existing one, for one thing. Additionally, I don't
think it's the correct thing to do even if it was done in a
source-compatible way, because it implies that there are two correct
formats for Hiccup, when in fact only one way will work when given as input
to Hiccup. I think the way to make this change is to add a new protocol,
as-hiccup-noescape or as-reagent or something, and keep it clear that its
output will not go into Hiccup correctly, nor will Hiccup's go into
Reagent's.


Reply to this email directly or view it on GitHub
#30 (comment)
.

@jeluard
Copy link
Collaborator

jeluard commented Nov 17, 2015

Moving to cljc sounds like a great idea for long term maintenance.

Upgrading ClojureScript version doesn't matter for ClojureScript users as their version will be used during compilation.
It does matter for Clojure users has newer ClojureScript require Clojure 1.7. Using cljc also introduces this dependency.

I don't think having a new release depending on Clojure 1.7 is a bad idea because hickory is pretty stable and users can always rely on previous releases. Also Clojure 1.8 is about to be stable and Clojure users have a tendency to upgrade rapidly.

It might be nice to introduce a change in hickory major version to make this clear.

@davidsantiago
Copy link
Collaborator

Ok, thanks for the feedback. I tend to agree that existing versions will continue to work for most users pretty well, but I always agonize about when the right time is to move new development to require a newer Clojure version, especially if it's not totally necessary. This does sound like good value, though.

@edvorg Thank you. When you resubmit, can you make two separate PRs, one for the updating of versions, and one for the updated version of the code? Thanks.

@edvorg
Copy link
Author

edvorg commented Nov 17, 2015

Yes sure @davidsantiago

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.

Make HTML escaping configurable
3 participants