Skip to content

Conversation

@SergeyPodgornyy
Copy link

Description

Improve readability of Support\Str::languageSpecificCharsArray()

Motivation and context

Associative array could be easier read and support as to different arrays, because you can always see which value refers to which value

How has this been tested?

All tests passed successfully

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)

Checklist:

  • I have read the CONTRIBUTING document.
  • My pull request addresses exactly one patch/feature.
  • I have created a branch for this patch/feature.
  • Each individual commit in the pull request is meaningful.
  • I have run tests to cover my changes.

{
$languageSpecific = static::languageSpecificCharsArray($language);

if (! is_null($languageSpecific)) {
Copy link
Author

Choose a reason for hiding this comment

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

https://github.com/kalessil/phpinspectionsea/blob/master/docs/probable-bugs.md#null-pointer-exceptions-prevention

is_object(), is_null() and similar is_*() functions calls are not analyzed, instead we recommend to rely on null identity and instanceof operators;

All 'is_null(...)' calls can be safely replaced with 'null === ...' constructs (or 'null !== ...' if the original construct was negated).

Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the framework uses is_null

Copy link
Author

Choose a reason for hiding this comment

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

@ntzm yes, but it will be more clear and correct to use null !== .... Should I revert this changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should stay consistent throughout the framework

Copy link
Author

Choose a reason for hiding this comment

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

@ntzm Rolled back

@sisve
Copy link
Contributor

sisve commented Feb 28, 2018

I understand what you're trying to do, but this part of the code is taken from the Stringy library. Your changes will be lost next time we update this method.

https://github.com/danielstjules/Stringy/blob/df24ab62d2d8213bbbe88cc36fc35a4503b4bd7e/src/Stringy.php#L1869-L1878

@SergeyPodgornyy
Copy link
Author

@sisve hm, good notice 👍 Should I contribute then to danielstjules/stringy?

@SergeyPodgornyy
Copy link
Author

Pull Request to original repository in progress danielstjules/Stringy#179

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