Skip to content

Enhance Yaml Component readme #17981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

theofidry
Copy link
Contributor

Q A
Branch 2.3
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR none

Users, though GitHub, Google or Symfony doc, may be redirected to the Yaml Component GitHub. While the lack of info in the readme is not that big of an issue, as we can guess there is more doc elsewhere and not that many users come there, it can still feel annoying or harmful for others.

This PR aims at proposing simple changes to provide a better experience to people coming on the repo by giving all the info they may require on it:

  • a badge to give the latest version published on composer
  • a link to the official and complete documentation
  • a quick guide on how to contribute to the project (gives the references to the Symfony Guide and how to run tests for the component)
  • a link to the changelog
  • a mention of the license

Also note that this could easily be enforced for other components/bridges as well: it requires minimal change without the risk of breaking any PR.

@dosten
Copy link
Contributor

dosten commented Mar 1, 2016

While I'm 👍 on improve the readme files of all components, I'm 👎 on this because applies to a single component; also we do not use badges in readme files (@fabpot already rejected this a while ago)

@@ -1,21 +1,33 @@
Yaml Component
==============

YAML implements most of the YAML 1.2 specification.
[![Package version](https://img.shields.io/packagist/v/symfony/yaml.svg)](https://packagist.org/packages/symfony/yaml)
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed, we are not using any badges on symfony/symfony.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's a policy on the repo I guess there is not much that can be done here. Still, do you have a means to display the latest composer version available automatically without the badge? Otherwise would a link to packagist be acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

well, you generally don't need to know it as composer require symfony/yaml will guess a constraint based on the available versions

@fabpot
Copy link
Member

fabpot commented Mar 2, 2016

adding a link to the docs is indeed a good idea. I would revert everything else (the changelog and the license for instance can be easily spotted as the README is displayed along side the main files in the repo).

I'm also in favor of removing the "How to run the tests part", which is probably not something people want to do anyway when discovering a new piece of software.

And indeed, this should be done on all components. Can you change this one only for YAML and when everyone is ok, you will be able to replicate on all other READMEs.

@stof
Copy link
Member

stof commented Mar 2, 2016

and this should probably be done in the 2.3 branch, to apply it to all versions


print Yaml::dump($array);
Contributing
Copy link
Member

Choose a reason for hiding this comment

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

I would keep such a section, but instead, its content should tell people to use the main repository when contributing (making it less likely to have to move PRs to the main repo, which breaks the development experience as people cannot modify the moved PR after that as it is owned by a core dev)

@theofidry
Copy link
Contributor Author

the changelog and the license for instance can be easily spotted as the README is displayed along side the main files in the repo

That would be the case if tests and source files where in folders, but here all the source files are at the root. The more source files there is, the harder it is to spot them.

Can you change this one only for YAML and when everyone is ok, you will be able to replicate on all other READMEs.

That's the plan, and will target the 2.3 branch as @stof mentioned. But I wanted to make sure everyone was ok with the change before applying it everywhere.


$array = Yaml::parse(file_get_contents(filename));
[See the Documentation](http://symfony.com/doc/current/components/yaml).
Copy link
Member

Choose a reason for hiding this comment

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

@fabpot @javiereguiluz should we use https links for the symfony.com website ?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So:

  1. I need to fix the link
  2. Wouldn't be possible to get an auto-redirection to HTTPS?

Copy link
Member

Choose a reason for hiding this comment

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

@theofidry symfony.com does not yet enforce HTTPS. but it is best to start using it, as it is available since a few months

@fabpot
Copy link
Member

fabpot commented Mar 3, 2016

Closing in favor of #17997

@fabpot fabpot closed this Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants