SINTEF / oteapi-optimade

A plugin repository for OTE API implementing OPTIMADE strategies and capabilities.
https://SINTEF.github.io/oteapi-optimade
MIT License
2 stars 0 forks source link

Corrected json serialisation #159

Closed jesper-friis closed 1 year ago

jesper-friis commented 1 year ago

Corrected JSON serialisation, JSON strings are UTF-8-encoded.

Please also make the JSON serialisation readable. Use two space indentation.

codecov-commenter commented 1 year ago

Codecov Report

Merging #159 (9449ffc) into main (973e8f2) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #159   +/-   ##
=======================================
  Coverage   76.19%   76.19%           
=======================================
  Files          15       15           
  Lines         605      605           
=======================================
  Hits          461      461           
  Misses        144      144           
Flag Coverage Δ
linux 76.19% <ø> (ø)
windows 82.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

jesper-friis commented 1 year ago

But changing the unit from Ångström to Å makes sense to me if pint will understand this 👍

Yes, pint understands "Å" and "angstrom".

jesper-friis commented 1 year ago

The byte characters you see are exactly UNICODE (utf-8), but I'd expect it to be manageable by any parser, as well as more safe for storing entities in any DB backend.

If you look at the diff, you see that the origin was the ASCII sequence "\u00c5ngstr\u00f6m". True, it is valid UTF-8, but is not in anyway understandable by neither humans or Pint.

jesper-friis commented 1 year ago

To be honest. The JSON files here are just because this wouldn't work natively if the entities were in YAML, which I'd much prefer.

DLite should not have problems with data models in YAML, you just have to provide them correctly to dlite.storage_path. Since we have standardised on JSON, dlite supports just appending a directory path with the JSON files to dlite.storage_path. For YAML you need to append it like "yaml:/some/path/*.yaml".

But it is much preferable to follow the standard way to work with DLite than inventing your own

CasperWA commented 1 year ago

The byte characters you see are exactly UNICODE (utf-8), but I'd expect it to be manageable by any parser, as well as more safe for storing entities in any DB backend.

If you look at the diff, you see that the origin was the ASCII sequence "\u00c5ngstr\u00f6m". True, it is valid UTF-8, but is not in anyway understandable by neither humans or Pint.

This is Unicode, just escaped - not ASCII. See https://www.ascii-code.com/character/%C3%A5

CasperWA commented 1 year ago

To be honest. The JSON files here are just because this wouldn't work natively if the entities were in YAML, which I'd much prefer.

DLite should not have problems with data models in YAML, you just have to provide them correctly to dlite.storage_path. Since we have standardised on JSON, dlite supports just appending a directory path with the JSON files to dlite.storage_path. For YAML you need to append it like "yaml:/some/path/*.yaml".

But it is much preferable to follow the standard way to work with DLite than inventing your own

It is much preferable to have a choice that is more easily available and usable than having to conform to outdated conventions.

jesper-friis commented 1 year ago

The byte characters you see are exactly UNICODE (utf-8), but I'd expect it to be manageable by any parser, as well as more safe for storing entities in any DB backend.

If you look at the diff, you see that the origin was the ASCII sequence "\u00c5ngstr\u00f6m". True, it is valid UTF-8, but is not in anyway understandable by neither humans or Pint.

This is Unicode, just escaped - not ASCII. See https://www.ascii-code.com/character/%C3%A5

Unicode "Å" is a sequence of 2-bytes (0x00, 0xc5). It is obviously not the same thing as the 6 byte sequence "\u00c5". The first is understood by Pint, the latter is not.

CasperWA commented 1 year ago

No longer relevant - fixes implemented instead in #169 and #170