Skip to content

Remove json library#498

Closed
mattytrentini wants to merge 1 commit intomicropython:masterfrom
mattytrentini:remove_json
Closed

Remove json library#498
mattytrentini wants to merge 1 commit intomicropython:masterfrom
mattytrentini:remove_json

Conversation

@mattytrentini
Copy link
Copy Markdown
Contributor

It's not clear why there's a python implementation of the json library - perhaps it pre-dates json as a full-featured built-in?

If there are no other benefits to using the built-in over the python implementation then we should remove the python implementation.

@stinos
Copy link
Copy Markdown

stinos commented May 2, 2022

At first sight this implementation implements pretty much everything CPython's implementation has i.e. much more arguments for the dump/load functions than MicroPython's builtin version. Not sure if anyone actually uses this though.

@mattytrentini
Copy link
Copy Markdown
Contributor Author

It actually looks like it is the CPython json implementation, albeit an older version.

It really would be good to know if people are using this! Then we could tell if it makes more sense to remove or update. I do think there's a strong case for the former...

@DusKing1
Copy link
Copy Markdown

DusKing1 commented May 3, 2022

I'm using this lib, please do not remove.

@mattytrentini
Copy link
Copy Markdown
Contributor Author

I'm using this lib, please do not remove.

Thanks for letting us know @DusKing1, can you also please explain what features of the library - more than the built-in json library - you're using?

At the moment there are no tests and if we're going to continue to support this library we ought to add some; it would make sense to preference the features that are being used.

@DusKing1
Copy link
Copy Markdown

DusKing1 commented May 3, 2022

I'm using this lib, please do not remove.

Thanks for letting us know @DusKing1, can you also please explain what features of the library - more than the built-in json library - you're using?

At the moment there are no tests and if we're going to continue to support this library we ought to add some; it would make sense to preference the features that are being used.

Oh, sorry I might misread this repo and PR. This is not the lib in the micropython firmware but add-on libs for device that are running mpy firmware right?

@dbrignoli
Copy link
Copy Markdown

We also rely on this JSON implementation, please do not remove it. Thank you.

@andrewleech
Copy link
Copy Markdown
Contributor

We also rely on this JSON implementation, please do not remove it. Thank you.

Hi @dbrignoli, do you use this copy of json that needs to be manually installed from micropython-lib? Not just the normal built in version?

If so, could you give some more details like #498 (comment) ?

@dbrignoli
Copy link
Copy Markdown

Hi @andrewleech,

A quick (partial) search of our codebase reveals we use indent and sort_keys kwargs to json.dump(). Additionally, I believe we encountered some "showstopper" issue with ujson which made us switch from ujson to json, but I cannot recall exactly what it was as it happened more than 2 years ago, sorry.

@andrewleech
Copy link
Copy Markdown
Contributor

@dbrignoli that's great thanks, I absolutely know what it's like to not remember how/why I fixed things years ago! - kwargs usage is a really helpful hint!

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.

5 participants