Rename file parameter to path in many FileAccess methods
#113217
Conversation
Mickeon
requested review from
a team and
bruvzg
and removed request for
a team
Mickeon
force-pushed
the
rename-FileAccess-file-parameters
branch
2 times, most recently
from
694fb16 to
54eee7e
Compare
Mickeon
force-pushed
the
rename-FileAccess-file-parameters
branch
from
54eee7e to
732ef48
Compare
|
|
||
| bool FileAccess::exists(const String &p_name) { | ||
| if (PackedData::get_singleton() && !PackedData::get_singleton()->is_disabled() && PackedData::get_singleton()->has_path(p_name)) { | ||
| bool FileAccess::exists(const String &p_path) { |
There was a problem hiding this comment.
Note that the exposed method already used path.
Mickeon
force-pushed
the
rename-FileAccess-file-parameters
branch
from
732ef48 to
c2ea770
Compare
| ClassDB::bind_static_method("FileAccess", D_METHOD("get_extended_attribute", "file", "attribute_name"), &FileAccess::get_extended_attribute); | ||
| ClassDB::bind_static_method("FileAccess", D_METHOD("get_extended_attribute_string", "file", "attribute_name"), &FileAccess::get_extended_attribute_string); | ||
| ClassDB::bind_static_method("FileAccess", D_METHOD("set_extended_attribute", "file", "attribute_name", "data"), &FileAccess::set_extended_attribute); | ||
| ClassDB::bind_static_method("FileAccess", D_METHOD("set_extended_attribute_string", "file", "attribute_name", "_data"), &FileAccess::set_extended_attribute_string); |
There was a problem hiding this comment.
Notice the typo _data here which has been overlooked.
|
|
||
| virtual BitField<UnixPermissionFlags> _get_unix_permissions(const String &p_file) = 0; | ||
| virtual Error _set_unix_permissions(const String &p_file, BitField<UnixPermissionFlags> p_permissions) = 0; | ||
| virtual BitField<UnixPermissionFlags> _get_unix_permissions(const String &p_path) = 0; |
There was a problem hiding this comment.
All these virtual methods will be confusing when their overrides are different
There was a problem hiding this comment.
I guess I can change them too at the cost of perceived noise in this PR.
There was a problem hiding this comment.
I'd say it's a good reason to avoid it
There was a problem hiding this comment.
Then I guess I will do nothing about it??
There was a problem hiding this comment.
IMO the confusion over "file" vs "path" is not critical enough to create such noise over, especially since IMO most of these cases are obvious in their behavior (Ironically I'd say only the hash methods actually are potentially confusing here)
|
Also what's the reason to leave the md5 methods as is that take a path? |
|
No reason, actually, I just forgot about them. These methods are exposed with the parameter named |
Mickeon
force-pushed
the
rename-FileAccess-file-parameters
branch
from
c2ea770 to
a2f7d85
Compare
|
Done and done. |
Mickeon
force-pushed
the
rename-FileAccess-file-parameters
branch
from
a2f7d85 to
8395f0c
Compare
There was a problem hiding this comment.
I won't block this but I don't think this is a worthwhile change, I don't expect any confusion to arise from these as they were previously and I don't think the noise is worth the hypothetical gain from the uniform naming and nominally clearer naming
If there were actual methods that did take some "file" value I'd say it would be different, but without that I'd say it's not critical. I'd restrict it to just the documentation side if we do anything
Are there any reports or posts on forums etc. indicating that people are running into issues due to the naming?
Edit: Especially since changing these files causes mass rebuild
Parameter name changes are never truly "critical", to be honest. By hoping any of them are, even smaller inconsistencies in the names can never be addressed.
I wouldn't mind just changing the exposed names because that would solve my pet peeve anyway. It'd just be inconsistent.
No. Good luck finding anything of the sort. These are the minor things that trip up developers just a bit, but not enough to talk about it on any social platform. Inconsistent terminology add to the general sense of unease and the more accessible we make the class reference (#91060, #113214), the more noticeable it is going to be. |
|
I'd say that the name alone isn't enough to cause confusion, it has to be a case where it's ambiguous if the parameter can be a file in the first place, I can't imagine almost any of these being of that kind, it would require users to assume that you can get this data from the contents of a file, which considering the range of expertise of users is possible but IMO extremely unlikely, and I'd say those users wouldn't be using those methods in the first place, at least not without reading the documentation So I don't think that these trip developers up at all |
2 participants
See #107536 (comment)
There are several methods in FileAccess whose parameter's name is
file. This is not just inconsistent, but misleading because these methods purely accept a path to a file.In a class all about accessing any given file's raw data, this distinction matters quite a lot, I feel.
It looks like these methods were added over time, each deriving from the last ( d454e1a 8aa6f29 85d3be8 ). They're quite advanced and certainly targeted towards tech-savvy users, so naming the path
filehere may come naturally to them. Other FileAccess methods just name the same kind of parameterpath, hence my belief that this is an oddity worth fixing.This PR renames the parameters in all affected methods from
filetopath. Each corresponding description in the class reference has been updated, as well.