search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] [WIP] Introduce FilePathUtils module #9348

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by le717 Sunday Feb 15, 2015 at 00:42 GMT Originally opened as https://github.com/adobe/brackets/pull/10600

Part of #7631.

This introduces a new FilePathUtils module, which moves many APIs from the FileUtils in an attempt to resolve some of the circular dependencies.

I'm mainly posting this now to get feedback, review what stays in FileUtils and what is moved to FilePathUtils, and resolve any remaining issues.

/cc@redmunds

le717 included the following code: https://github.com/adobe/brackets/pull/10600/commits

core-ai-bot commented 3 years ago

Comment by redmunds Monday Feb 16, 2015 at 16:20 GMT

@le717

  • Moving FileUtils.isStaticHtmlFileExt() and FileUtils.isServerHtmlFileExt() to LanguageManager will create a circular dependency between it and FilePathUtils during deprecation unless it is flat-out moved without redirect.

I suggested moving them to LanguageManager to get rid of the _staticHtmlFileExts and _serverHtmlFileExts hard-coded lists of file extensions. But, to make that work a new client vs server document field will need to be added to language data. I think a temporary, well-documented circular dependency to handle deprecation is OK, but it might be worth separating that change out to another issue.

core-ai-bot commented 3 years ago

Comment by le717 Thursday Feb 19, 2015 at 16:45 GMT

I've moved _is*FileExt() to LanguageManager, but left getSmartFileExtension() alone as moving it makes the circular dependencies a bit more convoluted:

Let me know what you want me to do.

Also, just a thought: what would you say to moving the FilePathUtils to the src/utils folder with the other modules that are classified as utils? it is one of the few utils modules that are not in that folder, and once the old FileSystem code is removed, only FileUtils and FilePathUtils will be in src/file.

core-ai-bot commented 3 years ago

Comment by redmunds Thursday Feb 19, 2015 at 18:22 GMT

@le717

getSmartFileExtension() requires LanguageManager

It sounds like getSmartFileExtension() belongs in LanguageManager. Does that work?

what would you say to moving the FilePathUtils to the src/utils folder with the other modules that are classified as utils?

Yes, I think that FilePathUtils belongs in the src/utils folder.

core-ai-bot commented 3 years ago

Comment by le717 Thursday Feb 19, 2015 at 19:18 GMT

Moving getSmartFileExtension() to LanguageManager would not effect the temporary circular dependency during deprecation (as it already exists), and would correctly rely on FilePathUtils. The second point would not exist. The only thing that could possibly stop it would be how that one file extension-related is in LanguageManager but all others are in FilePathUtils.

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Feb 19, 2015 at 19:31 GMT

This seems more disruptive than it needs to be. Couldn't we also break the circular dependency by just doing this:

During the deprecation phase, the cycle would persist of course, but after that it would go away.

The other cleanups seems separable, but could be incorporated here optionally:

It's not the cleanest thing in the world having a mix of utilities that work with paths vs. file contents in one module, but IMHO it's not a nasty enough issue to warrant deprecating over a dozen APIs some of which are probably used in a ton of extensions.

core-ai-bot commented 3 years ago

Comment by le717 Thursday Feb 19, 2015 at 19:38 GMT

@peterflynn

but IMHO it's not a nasty enough issue to warrant deprecating over a dozen APIs some of which are probably used in a ton of extensions.

In the back of my head I've been thinking the same thing, as I've been getting a ton of warnings from this branch with the 20 extensions I've installed, many from very common extensions. A new module might have made sense when the issue was filed, but probably not now.

Move getSmartFileExtension() to LanguageManager Leave everything else in FileUtils

I'm thinking so, yes, provided isCSSPreprocessorFile() is moved somewhere else at the same time (it too requires LanuageManager).

core-ai-bot commented 3 years ago

Comment by peterflynn Thursday Feb 19, 2015 at 21:06 GMT

Thinking about it a bit more, here are some possible ideas:

Thoughts?

core-ai-bot commented 3 years ago

Comment by le717 Thursday Feb 19, 2015 at 21:47 GMT

isCSSPreprocessorFile() -> CSSUtils getSmartFileExtension() -> LanguageManager, and maybe rename to something like getCompoundFileExtension() for clarity

I like these.

showFileOpenError() & getFileErrorString() -> ...maybe a new module like FileUI or FileDialogs or ErrorDialogs?

Perhaps if there are other error dialogs that can be easily relocated, an ErrorDialogs module in src/widgets might work.

_isStatic/ServerHtmlFileExt() -> LiveDevelopment Possibly change to be based on language id instead of file ext

Since future plans may effect this, I would go for making just these changes and filing a backlog card with extensibility ideas.

core-ai-bot commented 3 years ago

Comment by peterflynn Friday Feb 20, 2015 at 01:02 GMT

@le717 Sounds good to me.

Re the dialog, I guess there are actually two options:

core-ai-bot commented 3 years ago

Comment by le717 Friday Feb 20, 2015 at 18:34 GMT

Since making a whole new FilePathUtils module is too drastic to fix the issues, I've spun off the new changes to #10641 and am closing this.