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

Add tests borrowed from Hiccup's test suite and make small tweaks to match Hiccup/Reagent behavior. #4

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

alysbrooks
Copy link
Member

@alysbrooks alysbrooks commented Mar 20, 2023

So far, the only divergence that has a semantic difference is our handling of conflicts between attributes in the map and tag. I think we want to follow Reagent's handling, but I don't know what that is yet.

  • Verify it's okay to use Eclipse-licensed code in a project using the Mozilla Public License. I think these licenses are effectively identical, but IMO, it's worth double checking.
  • Divide failing tests into two buckets: skip and write up and issue (because we'll fix later) and amend and document (because we like how our behavior diverges).
  • Figure out how Reagent handles the conflicting scenario.

@alysbrooks alysbrooks marked this pull request as draft March 20, 2023 16:47
@alysbrooks
Copy link
Member Author

Based on looking into it further, MPL and EPL are very similar and as far as I can tell, can be combined freely.

@alysbrooks
Copy link
Member Author

Latest version of Reagent's test suite: https://github.com/reagent-project/reagent/blob/master/src/reagent/impl/template.cljs. While I'm just integrating Hiccup tests for now, I'm trying to avoid introducing any incompatibilities.

@alysbrooks alysbrooks changed the title WIP: Add tests borrowed from Hiccup's test suite. Add tests borrowed from Hiccup's test suite. May 26, 2023
@alysbrooks alysbrooks changed the title Add tests borrowed from Hiccup's test suite. Add tests borrowed from Hiccup's test suite and match Reagent behavior. May 26, 2023
@alysbrooks alysbrooks marked this pull request as ready for review May 26, 2023 20:56
@alysbrooks alysbrooks changed the title Add tests borrowed from Hiccup's test suite and match Reagent behavior. Add tests borrowed from Hiccup's test suite and make small tweaks to match Hiccup/Reagent behavior. May 26, 2023
@alysbrooks
Copy link
Member Author

I think we can merge this after #5.

@alysbrooks
Copy link
Member Author

I believe that #7 would have been caught by these tests.

@alysbrooks
Copy link
Member Author

alysbrooks commented Aug 1, 2023

Caught more issues introduced (I'm pretty sure) by #5:

  • If the class is specified in the initial keyword and the attribute map (e.g., [:div.foo {:class "bar"} "baz"]), it's now rendering twice, as "<div id="baq" id="bar" class="foo">baz</div>"
  • Fragments don't seem to work anymore. E.g., [:div [test-fragment-component "hello"]] gets rendered as "<div><<>nn><p>hello</p><p>hello</p></<>nn></div>"

The latter might be due to the interaction between #5 and this PR since I think the fragment test existed prior to this PR.

Never mind, that second issue was caused by a typo I introduced during rebase.

Alys Brooks added 8 commits July 31, 2023 23:49
For example, `['div]` and `["div"] are now allowed.

This ensures compatability with the original Hiccup and also Reagent
Document a few discrepancies I've noticed between our libraryy and
others.
We throw a different exception than Hiccup does.
This matches the behavior of Reagent and the original Hiccup.

For example, in `[:div#foo {:id "bar"}]`, the id would be bar and not
foo.
Our output differs slightly in ways that don't make any semantic
difference.
Revise wording and add bullet about `style` attribute.
Remove some issues that were in the original Hiccup tests.
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.

1 participant