googlefonts / pyfontaine

Python tool to check font files for language/character set support
https://github.com/googlefonts/pyfontaine
GNU General Public License v3.0
104 stars 20 forks source link

Use libary to output valid JSON #111

Closed alerque closed 4 years ago

alerque commented 4 years ago

Closes #110.

At least on my system, the module used by import json is in the base Python install, the same place the one for import csv used in the same location is, so I assume the is safe to use without adding dependencies to the project, but I am not super familiar with the Python ecosystem, so I'm open to suggestions if there is a better way to implement this.

Adding the 2 space indentation enables formatted output, which I think is much human friendly than the one line monster being output previously, but that's also just my preference. I'm happy to change this if somebody else feels strongly on the matter.

googlebot commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

alerque commented 4 years ago

Subject to the terms and conditions of this Agreement, You hereby grant to Google and to recipients of software distributed by Google a perpetual, worldwide, non-exclusive, no-charge, royalty-free, irrevocable copyright license to reproduce, prepare derivative works of, publicly display, publicly perform, sublicense, and distribute Your Contributions and such derivative works.

C'mon folks this project is listed as GPL3, what's this CLA nonsense? :disappointed:

@googlebot I signed it!

Under duress. :stuck_out_tongue_winking_eye:

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

alerque commented 4 years ago

Relevant bit of the output from the test suite, showing XML & JSON (starts on line 1274):

image

It's now both valid and readable.

alerque commented 4 years ago

Any possibility of getting this merged and released @vitalyvolkov or @davelab6? I have a couple projects held up waiting for this and I'm not looking forward to forking and providing a release of my own just so I can move them forward. This seems like a pretty cut and dry issue: the current output is clearly bogus and the fix is relatively simple to review. Is there something I could do to facilitate it getting merged & a new release being made available?

waldyrious commented 4 years ago

Any possibility of getting this merged and released @vitalyvolkov or @davelab6?

@vitalyvolkov doesn't seem to be active, but maybe @felipesanches can also take a look, as the only contributor to this repo in the past several months.

felipesanches commented 4 years ago

Obrigado por lembrar-se de mim, @waldyrious :-)

@alerque, I remember that I got a bit amusing by your slight rant about the CLA and ended up distracted and not taking action here. Sorry! I will review this now. Thanks for your contribution.

felipesanches commented 4 years ago

Oh! That's pretty straight-forward! LOL

Thanks again ;-D