citizenfx / fivem

The source code for the Cfx.re modification frameworks, such as FiveM, RedM and LibertyM, as well as FXServer.
https://cfx.re/
3.54k stars 2.08k forks source link

Bash Glob for resource file matching #1406

Open MrGriefs opened 2 years ago

MrGriefs commented 2 years ago

This issue proposes current 'glob' to be replaced with Bash Glob's syntax, including extglob and brace expansion.
For the sake of readability, Bash Glob syntax will be used in this issue to implicitly specify possible file matches in context.

Use case

Directory depth With current syntax, it is impossible to match client/*.js without matching client/html/*.js.

Complexity Globs can remain more consistent for all resources, allowing complex project structures with less entry chores.

{*.{C,c}lient.*,client/*.*} [Playground](https://globster.xyz/?q=%7B*.%7BC%2Cc%7Dlient.*%2Cclient%2F*.*%7D&f=manifest.lua%2CShared.Client.net.dll%2Cclient%2FCommands.js%2Cclient%2FEvents.js%2Cclient%2FMenu.js%2Cclient%2Fhtml%2Fapp.js%2Cclient%2Fhtml%2Findex.html%2Cserver%2FEvents.net.dll%2Cserver%2FSync.net.dll%2Csrc%2Fclient.lua%2Csrc%2FFeed.Client.net.dll%2Csrc%2Fserver.lua) ```diff resource - │ fxmanifest.lua + │ Shared.Client.net.dll │ ├───client + │ │ Commands.js + │ │ Events.js + │ │ Menu.js │ │ │ └───html - │ app.js - │ index.html │ └───server - Events.net.dll - Sync.net.dll ```
src/?(*.){C,c}lient.{lua,js,net.dll} [Playground](https://globster.xyz/?q=src%2F%3F(*.)%7BC%2Cc%7Dlient.%7Blua%2Cjs%2Cnet.dll%7D&f=manifest.lua%2CShared.Client.net.dll%2Cclient%2FCommands.js%2Cclient%2FEvents.js%2Cclient%2FMenu.js%2Cclient%2Fhtml%2Fapp.js%2Cclient%2Fhtml%2Findex.html%2Cserver%2FEvents.net.dll%2Cserver%2FSync.net.dll%2Csrc%2Fclient.lua%2Csrc%2FFeed.Client.net.dll%2Csrc%2Fserver.lua) ```diff resource - │ fxmanifest.lua │ └───src + client.lua + Feed.Client.net.dll - server.js ```

Caveat

The behaviour is not interchangeable and migration will be necessary for the majority of uses.

Migration

All 'glob' stars * are equivalent to one **/**. A 'glob' qmark ? is equivalent to {?,/}.

blattersturm commented 2 years ago

The behaviour is not interchangeable and migration will be necessary for the majority of uses.

Which makes it an instant not going to happen, and since discussion is out of scope for GH issues, post your request on the forums so it can be discussed.

MrGriefs commented 2 years ago

Isn't it possible to bump the manifest version and include a legacy 'glob' for prior versions? I've also already implemented this myself - with the intent of this issue being a draft to a potential PR. I suppose not

blattersturm commented 2 years ago

Alternately, adding brace handling - and maybe this question mark thing - to the existing logic would be compatible fine and not 'require everything to be changed', as, yes, sure, a manifest version bump could work fine but, no, that isn't desirable especially for cases where additional behavior can be added without breaking old behavior.

Similarly, as to the 'oh btw I have a PR pending', how come you don't mention that right away, even?! Code is usually seen as a lot more constructive to e.g. review than this which looked more like a feature request that also was awfully specific for something not discussed prior.

The initial post wasn't discernible as being anything but a request, a weird 'pls do this', or anything else at all as people have been using GH issues for weird things lately and the only reason they're opened is that GH doesn't let you restrict who can create new issues to e.g. contributors or team members only without removing and hiding the entire issue system...

... one example is this weird issue (#1402) which looked like a PR except it was a spam issue and not a PR and the same went for whatever this post initially said.

blattersturm commented 2 years ago

Also,

With current syntax, it is impossible to match client/*.js without matching client/html/*.js.

Huh? This should work fine, and if it doesn't that's a bug.

You should be able to do 'client/*.js' fine and only 'client/**.js' should include subdirectories too.

MrGriefs commented 2 years ago

Apologies, I'm use to creating issues instead of draft PRs on projects where feature requests are in scope. If I knew this was out of scope, I would've explicitly stated that I'm working on this!

In the matcher client/**.js creates client\/.*.*\/\.js, which definitely matches ([^/]*? would've been better). [Playground] citizen-resources-core/ResourceMetaDataComponent.cpp#L207 Also, I'm not an expert but / doesn't need to be escaped AFAIK.

blattersturm commented 2 years ago

I'm not sure where that regex part came from - I suppose that the main implementation for multiple stars should have been in MatchFiles below as this matcher portion ought to only run on a single level of a directory (cross platform compatibility for Windows' FindFirstFile patterns, I believe).

Maybe something got mixed up in an attempted fix at some other point in time.

I'll reopen this issue again so it'll remain in view, too, we're still trying to figure out a viable flow as to what-goes-where while at the same time onboarding a number of new 'internal' folks, so my apologies for the inconsistency and sudden reply trying to sweep things up: if you look at closed issues (not even counting some deleted issues!) there's a lot of weird being posted here.

MrGriefs commented 2 years ago

A contributor guidelines would be extremely helpful to new wannabe contributors like myself.

blattersturm commented 2 years ago

Issue seems abandoned and not actionable by us at this time.