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

#4132: Provide text_format render element #4133

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

stefan-korn
Copy link
Contributor

fixes #4132

  • Test coverage exists
  • Documentation exists

QA Steps

  • Add manual QA steps in checklist format for a reviewer to perform to confirm that the feature or fix is working. Include as much details as possible so that the reviewer doesn't lose time figuring out how to perform steps.

@stefan-korn
Copy link
Contributor Author

Hm, too many return statements in code climate ... seems 4 was acceptable, but 5 are not :-)

Should one switch to "switch" then?

@dafeder dafeder self-requested a review April 5, 2024 22:08
@dafeder dafeder self-assigned this Apr 5, 2024
Copy link
Member

@dafeder dafeder left a comment

Choose a reason for hiding this comment

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

Picking this back up - I can see where this rule is coming from in the coding standards. It makes it a little harder to read for there to be so many possible returns. I believe if you just have it just set a variable on each condition (call it $string or $return or whatever, and just return that at the end, you should be fine. You may get hit on code complexity for having so many conditions but I am willing to override that in codeclimate since it makes sense to handle this widget that way IMO.

What we will need to bring this in is, you guessed it, text coverage. You were able to provide good test imputs and assertions for what became #4212 do you think you could do the same here? Overall I very much agree with the approach you're taking here -- set it once in the UI schema and do not give the option on the form.

@dafeder dafeder assigned stefan-korn and unassigned dafeder Jul 3, 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.

Provide text_format render element for string schema properties
2 participants