Skip to content
New issue

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

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

Already on GitHub? # to your account

Add support of phpcs.dist.xml as well as .phpcs.dist.xml #371

Closed
wants to merge 1 commit into from

Conversation

olivervogel
Copy link

Description

Provide a better editor experience. As files should have a corresponding extension, as this is often used to recognize the file type and derive corresponding features such as highlighting.

This has already been done by many libraries with a similar setup.

Suggested changelog entry

Added support of phpcs.dist.xml & .phpcs.dist.xml configuration files.

Related issues/external references

None.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

To achieve a better editor experience, XML files should have a
corresponding extension, as this is often used to recognize the file
type and derive corresponding features such as highlighting.
@olivervogel olivervogel changed the title Added support of phpcs.dist.xml as well as .phpcs.dist.xml Add support of phpcs.dist.xml as well as .phpcs.dist.xml Feb 29, 2024
@stronk7
Copy link
Contributor

stronk7 commented Feb 29, 2024

Ref.: squizlabs/PHP_CodeSniffer#3744

@jrfnl
Copy link
Member

jrfnl commented Feb 29, 2024

Thank you @olivervogel for your interest in contributing to PHPCS.

I'm not strongly against this change, but I'm also not convinced of the need for it for the following reasons:

  1. There are already four configuration file names supported, including two using the .xml extension, so you can already use phpcs.xml for your distribution file and .phpcs.xml for local overload files and have editor support/syntax highlighting.
  2. The [.]phpcs.dist.xml file name(s) are already supported via --standard=phpcs.dist.xml, so why would these need to be auto-discovered ?
    If it is about contributors to the project knowing how to run PHPCS, that is something to document in, for instance, a composer script.
  3. In any decent editor, you can configure what extensions should be treated as what file type for the purpose of syntax highlighting and in most editors you can change this on the fly for each file with one click anyway, so again: why would this need support on the tooling level ?
  4. I have seen little interest in this feature so far (five likes in a year is not convincing).

In other words, the current argumentation is weak and what other tools do is of little concern and is not strengthening your case. Argue the case for this tool.

On a practical note: please do not open pull requests for something which needs discussion first. Please respect the CONTRIBUTING guide, in particular the section about new features.

As for the PR:

  • If this would be accepted, it will only be accepted for the 4.0 release, not before.
  • There are no tests covering this code/change.
  • I think a discussion would be needed about the file precedence order as I don't think this is the right order (and no argument is made why it should be this order)

@jrfnl
Copy link
Member

jrfnl commented Mar 10, 2024

Closing for lack of response.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants