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 new functions: pathListDir, pathIsFile, pathIsDirectory #3189

Merged
merged 31 commits into from
May 25, 2024

Conversation

TracerDS
Copy link
Contributor

@TracerDS TracerDS commented Sep 18, 2023

Also refactored SharedUtil a little (WIN32 might not be defined. _WIN32 will always be defined)

Syntax: table fileListDir ( string path ) table pathListDir ( string path )
returns table of entries on success, nil on failure

Additional functions

bool isFile ( string path ) bool pathIsFile ( string path )
Returns true if path is a file, false if a directory or path doesn't exist

bool isDirectory ( string path ) bool pathIsDirectory ( string path )
Returns true if path is a directory, false if a file or path doesn't exist

OOP functions

path.listDir => pathListDir
path.isFile => pathIsFile
path.isDirectory => pathIsDirectory


Resolves: #346

@lopezloo lopezloo added the enhancement New feature or request label Sep 19, 2023
@Pirulax
Copy link
Contributor

Pirulax commented Sep 20, 2023

Is this supposed to return a list of files in a directory?

@TracerDS
Copy link
Contributor Author

Is this supposed to return a list of files in a directory?

Relative to a current resource, yes

@botder
Copy link
Member

botder commented Oct 2, 2023

Is the Unix code block functional with if (!(dir = opendir("c:\\src\\")))?
Also, don't you exclude the directory items in the < C++17 block?
And last, how can the scripter differentiate between file and directory?

@TracerDS
Copy link
Contributor Author

TracerDS commented Oct 5, 2023

Is the Unix code block functional with if (!(dir = opendir("c:\\src\\")))? Also, don't you exclude the directory items in the < C++17 block? And last, how can the scripter differentiate between file and directory?

  1. Changed path to the actual path. Should be functional now
  2. Now including all files in a directory without adding the directory name before it too
  3. via isFile and isDirectory functions (comment edited)

@botder
Copy link
Member

botder commented Oct 8, 2023

Wouldn't it make more sense to append a directory separator for directory items?
Checking each item whether it's a directory or file seems expensive?

@TracerDS
Copy link
Contributor Author

TracerDS commented Oct 8, 2023

Wouldn't it make more sense to append a directory separator for directory items?

It might cause some issues when messing with directories directly via that function (unless mta takes care of that already)

Checking each item whether it's a directory or file seems expensive?

No, it's not really expensive.

If MTA doesn't have problems with directories ending with \ | / then sure, I can add a separator for directory entries

@Pirulax
Copy link
Contributor

Pirulax commented Oct 14, 2023

I don't think fileListDir makes a lot of sense as a name.
Perhaps something like getDirectoryEntries or (or, I'd prefer to separate them, getDirectoryFiles and getDirectoryFolders, both with a bool recursive parameter as well.
Or, could even go with pathGetFolders, pathGetFiles [if we want to follow the file prefix convention thingy]

@TracerDS
Copy link
Contributor Author

I don't think fileListDir makes a lot of sense as a name. Perhaps something like getDirectoryEntries or (or, I'd prefer to separate them, getDirectoryFiles and getDirectoryFolders, both with a bool recursive parameter as well. Or, could even go with pathGetFolders, pathGetFiles [if we want to follow the file prefix convention thingy]

its better to have one function that gets all entries that you can sort by using isFile/isDirectory functions.

@DNL291
Copy link
Member

DNL291 commented Oct 28, 2023

There's the module FileSystem which already does the same and has some more features/functions btw.
Although could be added nativelly yes if the only purpose is to get files from a directory instead of using the module just for that.

@TracerDS
Copy link
Contributor Author

There's the module FileSystem which already does the same and has some more features/functions btw.
Although could be added nativelly yes if the only purpose is to get files from a directory instead of using the module just for that.

Using module for that will decrease performance (since you have to load the module, load its functions and events not to mention modules are very limited and you cant use almost any MTA functions there at all without refactoring the whole mta server).
Also that would only make the command work in server scripts only.

Implementing it directly in MTA solves all these issues

@DNL291
Copy link
Member

DNL291 commented Oct 28, 2023

There's the module FileSystem which already does the same and has some more features/functions btw.
Although could be added nativelly yes if the only purpose is to get files from a directory instead of using the module just for that.

Using module for that will decrease performance (since you have to load the module, load its functions and events not to mention modules are very limited and you cant use almost any MTA functions there at all without refactoring the whole mta server). Also that would only make the command work in server scripts only.

Implementing it directly in MTA solves all these issues

Uhm I'm not sure if we really have these issues in the (FileSystem) module. But I do agree that having the functions client-side and being directly implemented into MTA is better.

@tederis
Copy link
Member

tederis commented Oct 29, 2023

There's the module FileSystem which already does the same and has some more features/functions btw.
Although could be added nativelly yes if the only purpose is to get files from a directory instead of using the module just for that.

Using module for that will decrease performance (since you have to load the module, load its functions and events not to mention modules are very limited and you cant use almost any MTA functions there at all without refactoring the whole mta server). Also that would only make the command work in server scripts only.

Implementing it directly in MTA solves all these issues

I doubt that modules can affect performance in a context of filesystem functions. The main disadvantage that modules have - they don't exist on a client side. So the only way to add proposed in this PR functions is to make them a part of MTA.

But I personally would not say that these functions are really needed on a client side. Typically, server(and therefore client scripts) know which files exist on a client and how to reach them, which makes sense from the security perspective.

Shared/sdk/SharedUtil.File.hpp Show resolved Hide resolved
Shared/sdk/SharedUtil.File.hpp Outdated Show resolved Hide resolved
Shared/sdk/SharedUtil.File.hpp Outdated Show resolved Hide resolved
Shared/sdk/SharedUtil.File.h Show resolved Hide resolved
@TracerDS TracerDS force-pushed the 180923_Add-fileListDir branch from 5ba94b2 to 9da8617 Compare December 8, 2023 14:58
@TracerDS TracerDS force-pushed the 180923_Add-fileListDir branch from 0407443 to c53dd92 Compare February 2, 2024 14:14
@TracerDS TracerDS changed the title Add new function: fileListDir Add new function: pathListDir Feb 2, 2024
@TracerDS TracerDS changed the title Add new function: pathListDir Add new functions: pathListDir, pathIsFile, pathIsDirectory Feb 2, 2024
@TracerDS TracerDS force-pushed the 180923_Add-fileListDir branch from ef5a412 to 316ebce Compare February 2, 2024 14:56
Lpsd and others added 10 commits February 5, 2024 16:41
[ci skip]
VS2022: 17.6.33815.320 => 17.6.33829.357

[skip ci]

This is an automated commit to keep track of toolchain changes on the build server.
It applies to every MTA build after this commit until further notice.
VS2019: 16.11.33801.447 => 16.11.33920.266

[skip ci]

This is an automated commit to keep track of toolchain changes on the build server.
It applies to every MTA build after this commit until further notice.
[ci skip]
VS2019: 16.11.33927.289 => 16.11.34031.81

[skip ci]

This is an automated commit to keep track of toolchain changes on the build server.
It applies to every MTA build after this commit until further notice.
Build Tools 2022 (17.7.34031.279)

This is an automated commit to keep track of toolchain changes on the build server.
It applies to every MTA build after this commit until further notice. [skip ci]
botder and others added 6 commits February 5, 2024 16:41
@TracerDS TracerDS force-pushed the 180923_Add-fileListDir branch from 242c3b9 to b2571d6 Compare February 5, 2024 15:49
Cannot remove `#if __cplusplus >= 201703L` because client side code is using these functions (multiplayer_sa is c++14)
@TracerDS TracerDS force-pushed the 180923_Add-fileListDir branch from 5cd45dd to 6d52a20 Compare March 25, 2024 18:42
`WIN32_TESTING` was changed to `_WIN32_TESTING`. Changed back to the original definition
Copy link
Contributor

@Nico8340 Nico8340 left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@Dutchman101
Copy link
Member

There's been many code reviews, so merging.
@TracerDS thanks, please add it to wiki though

@Dutchman101 Dutchman101 merged commit 74781c6 into multitheftauto:master May 25, 2024
6 checks passed
@TracerDS TracerDS deleted the 180923_Add-fileListDir branch May 26, 2024 09:25
MegadreamsBE pushed a commit to MegadreamsBE/mtasa-blue that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting files and directories in specified path