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

Utils(docs): fix incorrect case referencing an internal constant value of Linux OS #6628

Closed
wants to merge 1 commit into from

Conversation

Yexeed
Copy link

@Yexeed Yexeed commented Feb 15, 2025

The "linux" constant reference has wrong case ("linux" !== "Linux"). This got me in a micro stutter when looking at the doc.

Changes

This small change to a Utils::getOS() phpdoc doesn't seem to break anything, including BC or FC.

@Yexeed Yexeed requested a review from a team as a code owner February 15, 2025 15:16
@Yexeed Yexeed changed the title Utils: fix incorrect case referencing an internal constant value of Linux OS Utils(docs): fix incorrect case referencing an internal constant value of Linux OS Feb 15, 2025
@dktapps
Copy link
Member

dktapps commented Feb 15, 2025

The doc is correct:

public const OS_LINUX = "linux";

@dktapps dktapps closed this Feb 15, 2025
@Yexeed
Copy link
Author

Yexeed commented Feb 16, 2025

The doc is correct:

public const OS_LINUX = "linux";

@dktapps, I don't think you catch the point.

The constant OS_WINDOWS = "win", which is ALSO (e.g. doesn't has any impact to the constant value) told by the Utils::getOS doc:

* Windows => win

So when I've read the documentation of Utils::getOS, I get the knowledge that, if I have a runtime running on a Windows machine, I would get a "win" result from Utils:getOS. Which is correctly does what the doc says:

public const OS_WINDOWS = "win";

The documentation implies that
Windows machine => Utils::getOs() => Utils::OS_WINDOWS => "win"
That is correct!

Now, documentation of Utils::getOs() have another "result mapping":

* Linux => Linux

Thus, if I have a Linux machine, I would get a "Linux" (L uppercase) as a return value? No! :

public const OS_LINUX = "linux";

The actual result chain would be:
Linux machine => Utils::getOS() => Utils::OS_LINUX => "linux" (L is lowercase here!)

But the doc misleads me to that Linux machine returns a "Linux", but the actual is "linux", and this is the problem which this PR was going to resolve.

@dktapps
Copy link
Member

dktapps commented Feb 16, 2025

Ah, seems like I misread the diff. You are correct.

Though I think it really ought to be referring to the OS_* constants instead of hardcoded strings.

@dktapps dktapps reopened this Feb 16, 2025
@dktapps dktapps closed this in 9402a20 Feb 16, 2025
@dktapps dktapps added Type: Fix Bug fix, typo fix, or any other fix Resolution: Obsolete Superseded by other changes Category: Documentation Related to any documentation for PocketMine-MP labels Feb 16, 2025
@dktapps
Copy link
Member

dktapps commented Feb 16, 2025

I've addressed this separately. Thanks for bringing it to our attention :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Documentation Related to any documentation for PocketMine-MP Resolution: Obsolete Superseded by other changes Type: Fix Bug fix, typo fix, or any other fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants