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

Index from 1 #18

Closed
jennybc opened this issue Oct 25, 2016 · 14 comments

Comments

@jennybc
Copy link
Contributor

commented Oct 25, 2016

When using jsonedit() on an unnamed list, elements are labelled by index. Could these indices start at 1 for easier translation between the interactive view and the composition of [, [[, and map(my_list, INTEGER) commands?

screen shot 2016-10-24 at 10 41 55 pm

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Oct 31, 2016

I fear this one might be messy and require changes to the underlying jsoneditor. I generally try to avoid changes to the underlying library especially in the case of something as great, big, and complicated as jsoneditor. I will poke around to see what I find.

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Oct 31, 2016

... unless on the R side we name unnamed lists/arrays starting with 1

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Oct 31, 2016

@jennybc, as discussed above in #18 (comment), here is how we might handle on the R side.

recurse <- function(l, func, ...) {
  l <- func(l, ...)
  if(is.list(l) && length(l)>0){
    lapply(
      l,
      function(ll){
        recurse(ll, func, ...)
      }
    )
  } else {
    l
  }
}

add_number_names <- function(l){
  if(length(l)>0 && is.null(names(l))){
    names(l) <- seq_len(length(l))
  }
  l
}

listviewer::jsonedit(recurse(list(1:3), add_number_names))

image

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Oct 31, 2016

If we did this, I would think this should be optional behind an argument, since translation back to R will cause problems.

@jennybc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2016

Yes that would be great. Definitely agree re: optional. Naming the actual list currently has other unfortunate consequences for how, e.g., purrr operates. This is why I leave so many lists unnamed, when otherwise I would pull a name out of the list itself. But better labelling in the widget would be helpful.

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Oct 31, 2016

Personally, I try to minimize arguments to a function, and I am concerned jsonedit is reaching max arguments. Also, as I mentioned, I am planning to add a whole suite of potential viewers. Would adding a documented and exported helper function like number_unnamed be acceptable, or would you prefer it be an argument?

@jennybc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2016

I'd be happy to run the input through a helper first. I don't really expose the listviewer code in the tutorial anyway.

@jennybc

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2016

While we're discussing interface, I've thought that the name jsonedit() is a bit misleading. Why? First, I'm working with a list. Which may or may not have come from JSON. Second, I have no immediate plans to edit my list in this way and I'm not even sure I'd be on board with that, for the same reason I won't edit data by hand in Excel. Maybe the JS library you're wrapping sort of dictates the name? If not, I humbly submit listview() as an option worth considering.

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2016

I really like the idea of listview. Originally, I had intended to provide a separate function for each viewer and then have a wrapper function like listview. We did something similar in DiagrammeR with mermaid and viz.js. I struggle though since jsoneditor is so amazing (and getting a full rewrite) that I might just dedicate listviewer to it. Tough decisions :)

timelyportfolio added a commit that referenced this issue Nov 1, 2016
timelyportfolio added a commit that referenced this issue Nov 1, 2016
@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2016

I have added a number_unnamed helper function. Once I hear feedback, I will push the submit to CRAN button. Thanks again.

@jennybc

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2016

I've tried number_unnamed(). The R-like indexing from one is great. But something else has changed that is less desirable. Not sure if same root cause?

Here's before, I which we can see the sub-component names and values?

screen shot 2016-11-01 at 7 36 47 am

Here's after:
screen shot 2016-11-01 at 7 37 05 am

See how all the sub-components are de-expanded now and I would need to click on each one to reveal its single unit of info?

Is it possible to have the indexing from 1 and not this other behaviour?

@jennybc

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2016

This does seem to have something to do with running the primary input list through number_unnamed(). If I revert (and tolerate the display of 0-based indices), the sub-components display compactly again.

timelyportfolio added a commit that referenced this issue Nov 1, 2016
@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Nov 1, 2016

Makes sense, I had set the check to length(l) > 0 which will assign numbers to unnamed single element list/vector. I just changed the check to length(l) > 1 which will mean we don't get a name on single element list/vector. Will you please verify on your end? Thanks!

Here is my test case.

l <- list(
  list(
    a="a",
    b="b"
  ),
  list(
    a="a",
    b="b"
  )
)

jsonedit(l)

jsonedit(number_unnamed(l))

image

@timelyportfolio

This comment has been minimized.

Copy link
Owner

commented Jan 10, 2017

going to close this; please let me know if it should be reopened. Thanks so much for the great idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.