Skip to content

Mandate double quotes around phone numbers#71

Merged
libremente merged 6 commits intomasterfrom
fix-70
Oct 10, 2019
Merged

Mandate double quotes around phone numbers#71
libremente merged 6 commits intomasterfrom
fix-70

Conversation

@libremente
Copy link
Copy Markdown
Member

Content

This PR mandates to insert the phone number as a string. As such, it mandates to explicitly insert double quotes around the phone number.

Review

  • Ensure your files are written following RST specs (not MD!)
  • Italian version
  • English version
  • Example files
  • Ask for review

Copy link
Copy Markdown
Member

@sebbalex sebbalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread docs/en/example/publiccode.yml Outdated
email: "francesco.rossi@comune.reggioemilia.it"
affiliation: Comune di Reggio Emilia
phone: +39 231 13215112
phone: "+39 231 13215112"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already a string as it contains spaces, and doesn't need quotes.
I am a fan of making things more clear, so I am positive about this change, I am simply pointing out that it's not a real change of the specifictation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I thought that this could be the extreme example: if we put double quotes also on a string which is already interpreted as a string in YAML we are passing the idea that it must be explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We observed several cases where phone numbers had different format, see below:

>>> yaml.safe_load('phone: 01234567')
{'phone': 342391}

>>> yaml.safe_load('phone: 023113215112')
{'phone': 2569869898}

>>> yaml.safe_load('phone: +39023598745')
{'phone': 39023598745}

That's why we need quoting in specification.

go-yaml/yaml#86 (comment)

ref

Copy link
Copy Markdown
Contributor

@alranel alranel Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with both of you :-)
We can't mandate the usage of quotes, because that depends on the rules of YAML and the heuristics of the libraries. But it is useful to use double quotes in our examples because that reminds to people who write publiccode.yml files by hand that it's better to use double quotes. Otherwise we should explain "this example does not use quotes because it contains spaces". People are not that expert of YAML :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What @alranel wrote is precisely what I meant! Let's keep quotes for clarity but, technically, that field was already a string

Comment thread docs/en/schema.core.rst Outdated
specification.
- ``phone`` - phone number (with international prefix)
- ``phone`` - phone number (with international prefix). This has to be
a string delimited by double quotes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the requirement of it being delimited by double quotes, it is an implementation detail and not the only way to properly represent strings in yaml

Comment thread docs/it/schema.core.rst Outdated
siccome questo è permesso dalle specifiche YAML.
- ``phone`` - Numero telefonico (con prefisso internazionale).
- ``phone`` - Numero telefonico (con prefisso internazionale). Questa chiave
deve essere una stringa delimitata da doppi apici.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Copy Markdown
Member

@ruphy ruphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked the changelog to reflect the doc... and I think we're ready for a 0.2.1!

@libremente
Copy link
Copy Markdown
Member Author

Thanks @ruphy @sebbalex @alranel! Merging! 👍

@libremente libremente merged commit 68b56af into master Oct 10, 2019
@libremente libremente deleted the fix-70 branch October 10, 2019 14:11
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