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

Rename OS to Platform #47789

Closed
wants to merge 1 commit into from

Conversation

madmiraal
Copy link
Contributor

As originally identified by @leonkrause here, not every platform is an OS, but every OS is a platform, and it's how we refer to each of the platforms that inherit from this class too. Therefore, it makes sense to rename OS to Platform. To make everything consistent, this PR includes renaming all the inheriting classes too.

Part of #16863.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #41091, #47370, #47701 and #47936.

@neikeq
Copy link
Contributor

neikeq commented Apr 17, 2021

I don't like this change because Platform takes longer to type and takes more horizontal space. IMO consistency is not worth it in this case.
If we look at the members of the OS class, many if not most of them don't belong to an OS or Platform class, but rather other concepts like thread, environment, process, system, etc. This class provides too many methods for vastly unrelated things and it would be better for it to be split. That may be hard though without support for static methods (e.g.: delay_msec/usec should be static methods of Thread).

@Xrayez
Copy link
Contributor

Xrayez commented Apr 17, 2021

This class provides too many methods for vastly unrelated things and it would be better for it to be split. That may be hard though without support for static methods (e.g.: delay_msec/usec should be static methods of Thread).

That what I thought as well, there needs to be better separation. There are a number of proposals which also suggest static methods support, like godotengine/godot-proposals#1101.

So I think it's fine to introduce Platform singleton, but it should be limited to its scope.

@Chaosus
Copy link
Member

Chaosus commented Apr 18, 2021

I don't like this change because Platform takes longer to type

I feel similarly, Maybe PM or PF sounds better?

@Xrayez
Copy link
Contributor

Xrayez commented Apr 18, 2021

I don't like this change because Platform takes longer to type

I feel similarly, Maybe PM or PF sounds better?

I don't think the verbosity is the major concern for rename. OS is recognized by nearly everyone who does programming, while shortening the name to PM (project manager?) or PF will likely be confusing to new contributors.

@Chaosus
Copy link
Member

Chaosus commented Apr 18, 2021

I don't like this change because Platform takes longer to type

I feel similarly, Maybe PM or PF sounds better?

I don't think the verbosity is the major concern for rename. OS is recognized by nearly everyone who does programming, while shortening the name to PM (project manager?) or PF will likely be confusing to new contributors.

And what do you think about preserving both OS and Platform as a reference to the same singleton?

@aaronfranke
Copy link
Member

aaronfranke commented Apr 18, 2021

On one hand, it makes sense because not every platform is an OS (ex: web), so "Platform" is more technically correct. On the other hand, "OS" is shorter, very recognizable, and it's also unambiguous ("Platform" can also refer to things like Steam vs Itch vs Epic).
There is also the downside of compatibility breakage, which makes it very tempting to just not change this.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 20, 2021

@Chaosus: And what do you think about preserving both OS and Platform as a reference to the same singleton?

To quote reduz on the topic of having two APIs for the same thing:

@madmiraal
Copy link
Contributor Author

Rebased following merge of #47343, #47398, #47448, #47917, #47974, #47990, #48050, #48098, #48185 and #48292.

@madmiraal
Copy link
Contributor Author

Rebased following merge of #47772.

@nathanfranke
Copy link
Contributor

Rebased following merge of #47343, #47398, #47448, #47917, #47974, #47990, #48050, #48098, #48185 and #48292.

You can just re-base it, there's no need to ping every single PR that modifies something OS-related.

@akien-mga
Copy link
Member

We discussed this in a PR review meeting today, and the consensus was that this rename doesn't seem like a good idea anymore.

Leon's proposal made sense for the 3.x codebase as the OS class did pack a lot of API which is platform-specific more than OS-specific (and the code folders are indeed called platforms/<name>/). But since 4.0, the platform-specific APIs are split in two classes, OS and DisplayServer, with a much cleaner separation of concerns. "Platform" is the union of OS and DisplayServer, and the "new" OS is indeed mostly OS-specific APIs, so the names make sense. Changing OS to Platform would actually make things more ambiguous now.

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.

7 participants