Skip to content

Conversation

@philipshurpik
Copy link
Contributor

@ebdrup We faced with problem that in debitoor app we need bigger count limit for properties.
What do you think about this?
Should we make it configurable?
Or just increase default count value?
cc @bubenshchykov

@bubenshchykov
Copy link
Collaborator

just by looking at code - it's a magic number for me, i don't know how previous "100" was picked up

if 1000, does it mean that we can have up to 1000 props in failed object? or in array? why not 100000 then? :)

@ebdrup
Copy link
Owner

ebdrup commented Feb 22, 2016

It was picked so the test don't cause the process to crash. Express puts the http/app object on some errors that caysed a crash.
Feel free to do a PR with green tests ;-)

On 22. feb. 2016, at 18.04, Alex Bubenshchykov notifications@github.com wrote:

just by looking at code - it's a magic number, i don't know how previous "100" was picked up

does it mean that we can have up to 1000 props in failed object? or in array? why not 100000 then? :)


Reply to this email directly or view it on GitHub.

@eagleeye
Copy link
Collaborator

@ebdrup this PR is green) We had a real case with real object of 300 entities. It was credit note with 10 invalid lines. Fork is used in debitoor app, so we are going just to increase this magic number to 1000 for now.

@ebdrup
Copy link
Owner

ebdrup commented Feb 23, 2016

You have the rights to merge an deploy to npm AFAIR.

On 23. feb. 2016, at 09.32, Andrii Shumada notifications@github.com wrote:

@ebdrup this PR is green) We had a real case with real object of 300 entities. It was credit note with 10 invalid lines. Fork is used in debitoor app, so we are going just to increase this magic number to 1000 for now.


Reply to this email directly or view it on GitHub.

eagleeye added a commit that referenced this pull request Feb 23, 2016
adjusted serializable object properties count tp 1000
@eagleeye eagleeye merged commit 12f90d7 into ebdrup:master Feb 23, 2016
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