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

Ensure ToDynamic is called consistently when converting a Element(s) #266

Closed
wants to merge 2 commits into from
Closed

Ensure ToDynamic is called consistently when converting a Element(s) #266

wants to merge 2 commits into from

Conversation

kylejuliandev
Copy link

@kylejuliandev kylejuliandev commented Apr 23, 2024

Motivation

Fixes #265

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

* Simplify FromDynamic conversion for TaxonomyElement
* Fix unit test
* Attempted to do RichTextElement but it broke too many tests, so may need further thought
* Make ToDynamic more consistent in the Element types
* Improve unit test coverage
@kylejuliandev kylejuliandev marked this pull request as ready for review May 31, 2024 22:21
@kylejuliandev kylejuliandev requested review from pokornyd and a team as code owners May 31, 2024 22:21
@kylejuliandev
Copy link
Author

I made an attempt at aligning RichTextElement but it proved to be tricky. Lots of tests assume the dynamic structure in different places, so after the refactor ~70 tests began to fail. I reverted them for now, maybe an opportunity for later?

@kylejuliandev kylejuliandev changed the title Ensure ToDynamic is called when converting a TaxonomyElement Ensure ToDynamic is called consistently when converting a Element(s) May 31, 2024
@pokornyd
Copy link
Member

hey, thank you for the submission and an update regarding the tests. I'll review the proposed changes and see about refactoring the tests or splitting it into a separate issue.

@kylejuliandev kylejuliandev closed this by deleting the head repository Jul 19, 2024
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.

FromDynamic on TaxonomyElement produces RuntimeBinderException on Value
2 participants