Skip to content

Change field separator to double-underscore, defined as a constant#688

Closed
ghost wants to merge 1 commit intorjsf-team:masterfrom
echee2:Make-field-separator-a-constant
Closed

Change field separator to double-underscore, defined as a constant#688
ghost wants to merge 1 commit intorjsf-team:masterfrom
echee2:Make-field-separator-a-constant

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Aug 30, 2017

Reasons for making this change

idSchema.$id currently uses _ to separate fields. Some apps need to parse id (e.g. to navigate a redux global state tree).

Problem: Having a single underscore as field separator prevents form field names from containing underscore -- non-field-separating underscores are confused with actual field separators.

While JS style is camelCase, may be too rigid a constraint. E.g. for our current project, we are attempting to port a python app which liberally uses underscores, and forcing field name changes to exclude underscore is too imposing.

Proposed solution

Replace single underscore with double underscore. Although a double-underscore is still somewhat arbitrary, it should accommodate the majority of cases.

Changes made

This is an internal change. Thus, the primary goal was to ensure all existing tests pass.

Besides the double-underscore change, the only other diffs were formatting changes introduced by Prettier (npm run cs-format). Seems strange that it would cause so many formatting diffs?

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@n1k0
Copy link
Copy Markdown
Collaborator

n1k0 commented Aug 31, 2017

Dunno what version of prettier you did use but could you please rebase against latest master and ensure 1.6.1 is installed and used to format your code? That would greatly help reviewing this diff.

@ghost ghost force-pushed the Make-field-separator-a-constant branch from fb05b5a to c46cbc0 Compare August 31, 2017 16:56
@ghost
Copy link
Copy Markdown
Author

ghost commented Aug 31, 2017

@n1k0 thanks for the guidance. Prettier 1.6.1 was already in use. Rebasing seems to have done the trick. Only the relevant diffs now show (except for a couple of Prettier reformats, due to line length increase).

@glasserc
Copy link
Copy Markdown
Contributor

glasserc commented Sep 6, 2017

It seems like we don't describe idSchema or the generated IDs in any way in our README, which is the closest thing we have to an interface contract, so I guess that means we don't have to worry about breaking backwards compatibility. It might be nice to add a note saying "DO NOT RELY ON THE FORMAT OF THE GENERATED IDS" if we really intend for this to be true, though. The PR looks good and I'll merge it unless @n1k0 has any last thoughts?

@olzraiti
Copy link
Copy Markdown
Contributor

olzraiti commented Sep 12, 2017

Could we parameterize the separator and keep single underscore as default? That way we could keep backward compability. Also, there could be users with fields that have two underscores in field names (not likely, but possible). I rely heavily on the current idSchema format. Although idSchema is not documented, it is very useful for targeting DOM elements and quite likely I'm not the only one using idSchema.

@suriya-p-rivet
Copy link
Copy Markdown

Hi Guys, Any update on this?

@epicfaace
Copy link
Copy Markdown
Member

epicfaace commented Mar 5, 2019

I think, as @olzraiti said, it would make sense to keep it using one underscore as the delimiter, but keep the option to change the delimiter as an option; perhaps it could be passed in as a prop. If you can make a PR based off of this one that does this, I'd be ready to merge it!

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.

6 participants