Skip to content
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

Add support for checking if a language is supported. #84

Closed

Conversation

hopeseekr
Copy link
Contributor

No description provided.

@hopeseekr hopeseekr changed the title Add support for checking if a lanugage is supported. Add support for checking if a language is supported. Apr 1, 2024
@coveralls
Copy link

coveralls commented Apr 1, 2024

Pull Request Test Coverage Report for Build 8510621551

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 94.249%

Totals Coverage Status
Change from base Build 8493129858: 0.004%
Covered Lines: 1180
Relevant Lines: 1252

💛 - Coveralls

@aidan-casey
Copy link
Member

@hopeseekr - It looks like the style check is not passing. Do you mind running composer qa and re-pushing?

@@ -52,6 +52,11 @@ public function __construct(
}
}

public function isSupportedLanguage(string $language): bool
Copy link
Member

Choose a reason for hiding this comment

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

Given the naming of the other methods, I wonder if it would be more intuitive to call this hasLanguage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. done.

{
$highlight = new Highlighter();

Assert::assertTrue($highlight->hasLanguage('php'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For about 10 years, from PHPUnit 1.0 in 2006 until PHPUnit 5.0 in Oct 2015, PHPUnit needed to use $this->assert() methods.

However, with the adoption of PHP 5.6 and the new static mechanism, it offloaded all of the assert functions to a standalone class PHPUnit\Framework\Assert, which later (in the PHPUnit 9 days) became its own packagist repo in its own right.

Calling static methods as regular instance methods is considered a bad practice. Because of this, with the release of PHPUnit 7.0, in 2019, there was a discussion about whether one should still use $this-> or self:: for the methods.

Two camps emerged: If you use static analysis, believe in strong typing, then prefer self::. If you prefer the dynamic nature and loose typing of PHP in a by-gone-tho-still-supported era, use $this->.

Because the php-cs-fixer of this project enforces $this->, I've decided to just call the Assert class directly. It's faster and directly equivalent to self::, so it's actually even better, from a technical standpoint.

Copy link
Member

Choose a reason for hiding this comment

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

@hopeseekr - This has been addressed here and the stance of this project will not change based on your assertions of best practice, particularly on such a trivial thing. The php-cs-fixer represents the style choices made and exists for a reason.

Contributions are welcomed and valued, but pushing your own opinions upon the project is not helpful to forward progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I apologize and will not repeat the mistake.

@aidan-casey aidan-casey closed this Apr 1, 2024
@hopeseekr hopeseekr deleted the is_supported_lanuage branch April 1, 2024 18:10
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.

3 participants