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

enableNestedDataAccess option to disable nested data when needed #909 #1181

Merged
merged 4 commits into from
Jun 2, 2020

Conversation

domind
Copy link
Contributor

@domind domind commented Feb 4, 2020

also added 2 tests for option enableNestedDataAccess : false :

  1. making sure that table is not generated same way like when with enableNestedDataAccess : true
  2. table is correctly omitting nested data.

@coveralls
Copy link

coveralls commented Feb 4, 2020

Coverage Status

Coverage increased (+0.07%) to 76.508% when pulling 60753f5 on domind:fix-for-#909--nested-data-access into 4f83609 on gregnb:master.

@gabrielliwerant
Copy link
Collaborator

Hello @domind. Can you explain the use case for shutting off this functionality via an option? It was my thinking that one would simply not use the dot notation if he/she did not want to use nested data.

@domind
Copy link
Contributor Author

domind commented Feb 12, 2020

Hi @gabrielliwerant you proposed to add this as an option

I'm open to suggestions about how to work around this. It's a little messy, but we could add an option like enableNestedDataAccess which could be flagged on or off to enabled/disable parsing the dot.

Right now, by default nested data are enabled, so if there is a dot in name data are not displayed.

@gabrielliwerant
Copy link
Collaborator

@domind Ok, thanks for reminding me. So, another way to do this would be to allow an option for choosing which character to use as a separator for objects. The advantage of this approach would be that there would be a way for someone to use this feature even if there are dots in the names, instead of having to choose between enabling/disabling the feature, What do you think?

@domind
Copy link
Contributor Author

domind commented Feb 13, 2020

Sounds reasonable. Should we still keep name "enableNestedDataAccess" in such case? Guess default will be enabled with dot as separator, but should we allow to disable it completely?
Might be to rename it to nestedDataSeparator and it would take string value, and "disable" string would turn it off. Should I rewrite it that way?

…d data can use any separator, default value if this option is omitted is '.' Passing empty string disables nested data access.
@domind
Copy link
Contributor Author

domind commented Feb 17, 2020

@gabrielliwerant - arranged following:

  • changed enableNestedDataAccess to string, so now nested data can use any separator,
  • default value in case option is omitted is set to '.' (processing of nested data enabled)
  • to disable - you need to pass empty string enableNestedDataAccess : ''
  • corrected test cases

@patorjk patorjk changed the base branch from master to v3 June 2, 2020 02:23
@patorjk patorjk merged commit f6f1fa2 into gregnb:v3 Jun 2, 2020
@patorjk patorjk mentioned this pull request Jun 2, 2020
@patorjk
Copy link
Collaborator

patorjk commented Jun 2, 2020

Thank you for putting this together!

I've gone ahead and merged this into v3 and added some documentation in the README.

This feature was previously mentioned only in one example, and that was for something else and wasn't even linked on the README page. So it was kind of a hidden feature. I actually think having it on by default is bad since it leads to confusing bugs as seen in #909. I think I'm going to set it to be off by default in v3. It's a breaking change, but I'll have it documented in the write-up for v2 to v3.

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.

4 participants