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

Return of experiment with big shiny app #275

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

yannrichet
Copy link

  • add test for loadTimeout
  • add testApp(...,normalizeContent=T, ...) option to disable false diff detected when json content order changes.

@jcheng5
Copy link
Member

jcheng5 commented Aug 12, 2019

Thank you for the contribution!

First, there seems to be a bug in this line: if (length(names(l))) return(empty.list), I'm guessing this should be if (length(names(l)) == 0)? Currently, I'm just getting empty lists back.

Also, just to be clear about the intention here, the normalization (actually I think the more commonly used word for this is "canonicalization") is to find all JSON objects (i.e. key/value pairs) and order their elements by key? This code does so recursively, but only objects directly nested within objects--I don't think objects inside arrays are covered (or objects inside of arrays of arrays, or arrays of objects of arrarys of objects...).

I assume this won't work (the cc and bb won't be reordered):

order.list(list('d'=list(list('cc'=1,'bb'=0)),'a'=1,'b'=2,'c'=3))

@yannrichet
Copy link
Author

Thank you for the contribution!
Thank you for your work! :)

First, there seems to be a bug in this line: if (length(names(l))) return(empty.list), I'm guessing this should be if (length(names(l)) == 0)? Currently, I'm just getting empty lists back.

Yes, sorry.

Also, just to be clear about the intention here, the normalization (actually I think the more commonly used word for this is "canonicalization")

ok. Do you wish I change the option name ? (to be honest I also feel it is not so clear for now...)

is to find all JSON objects (i.e. key/value pairs) and order their elements by key? This code does so recursively, but only objects directly nested within objects--I don't think objects inside arrays are covered (or objects inside of arrays of arrays, or arrays of objects of arrarys of objects...).

Well, you are right. Again.
Also, I still did some mistake in this PR, so I still have pushes to finish...

I assume this won't work (the cc and bb won't be reordered):

order.list(list('d'=list(list('cc'=1,'bb'=0)),'a'=1,'b'=2,'c'=3))

No, I think it will reorder:

order.list(list('d'=list('cc'=1,'bb'=0),'a'=1,'b'=2,'c'=3))
$a
[1] 1

$b
[1] 2

$c
[1] 3

$d
$d$bb
[1] 0

$d$cc
[1] 1

but this one will not (and will even loose some data):

order.list(list('d'=c(list('cc'=1,'bb'=0),'aa'),'a'=1,'b'=2,'c'=3))
$a
[1] 1

$b
[1] 2

$c
[1] 3

$d
$d$bb
[1] 0

$d$cc
[1] 1

So, I will fix that.

@yannrichet yannrichet changed the title Return of experiment with big shiny app WIP: Return of experiment with big shiny app Aug 13, 2019
@yannrichet
Copy link
Author

I think it is fixed now. At least, unnamed elements are no longer lost...

@yannrichet yannrichet changed the title WIP: Return of experiment with big shiny app Return of experiment with big shiny app Aug 13, 2019
@yannrichet
Copy link
Author

Hi. I am still waiting for review about that, if you have some time... No problem if you want some improvement or refactoring of code, but I would really like to have these features integrated in mainstream version...
Regards.

@CLAassistant
Copy link

CLAassistant commented Sep 14, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@bbax94
Copy link

bbax94 commented Apr 23, 2020

I believe I am also experiencing the same issue this pull request is addressing.

When running devtools::test() all my tests pass but when running through devtools::check() the current json file is not in alphabetical key order (unlike the expected file). Running through a json comparison tool will identify the files are the same but a simple text comparison will fail as the keys are now not in order.

I assume this is a similar issue to what @yannrichet was experiencing and trying to solve.

Is there any update on this pull request and when we can expect it to be merged and released?

@yannrichet
Copy link
Author

At least, you can test my fork of shinytest. This could confirm it solves your issue.

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