-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Better compartmentalize code that assumes no trailing "/" #5527
Conversation
- Rename FileUtils.canonicalizeFolderPath() to stripTrailingSlash() since it actually makes paths NOT canonical. Old API left in place with deprecation warning for now. - Add warning to docs for a few other APIs that return non-canonical paths: FileUtils.getNativeBracketsDirectoryPath(), FileUtils.getNativeModuleDirectoryPath(), ProjectManager.getInitialProjectPath(), ProjectManager.updateWelcomeProjectPath() ProjectManager._getWelcomeProjectPath() - Indicate explicitly which ProjectManager APIs support receiving non-canonical paths - Centralize code that touches welcomeProjects storage pref for clarity What this does NOT do: - Fix the other APIs listed above that still return non-canonical paths - Fix non-canonical first-launch project path given to _loadProject() - Fix non-canonical paths used in RecentProjects prefs - Fix non-canonical paths used in welcomeProjects pref - Fix ScopeManager, which in some places requires canonical paths and in other places requires non-canonical paths
… non- canonical: - ProjectManager.getInitialProjectPath() - always returns canonical - ProjectManager._getWelcomeProjectPath() - always returns canonical - ProjectManager.updateWelcomeProjectPath() - arg assumed to be canonical & always returns canonical None of these APIs are used in extensions. This does NOT change the underlying "welcomeProjects" pref - like "recentProjects", it still stores paths without trailing "/" under the hood
|
@njx You might want to look at this if you have time, since it largely concerns the "welcome project" detection and the RecentProjects extension -- both of which I think you've worked on. |
src/file/FileUtils.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to check the length here? Seems that we should make sure it isn't an empty string and is truthy before invoking the index operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrays in JS aren't length checked -- you just get undefined for outside of range indices. So the equality check is guaranteed to correctly evaluate to false for an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll re-add it though just so it's more explicit
|
Done with initial review. just a few minor nits. |
…string behavior more explicit
|
@JeffryBooher Changes pushed -- let me know if you need me to address anything else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why this isn't a public function in FileUtils.js? Seems handy to have for other modules/extensions to be able to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeffryBooher I'm hoping we won't need to get people in the habit of manually normalizing paths. Even in Brackets today, folder paths from almost every API are guaranteed to end in "/" -- these ones ProjectManager deals with are among the few exceptions. And the newer file API we're working on will make that guarantee even more explicit.
|
Looks good. Unit/Integration tests pass. Merging |
Better compartmentalize code that assumes no trailing "/"
|
Thanks @JeffryBooher! |
The "canonical" folder path format used in DirectoryEntry.fullPath includes a trailing "/". However, we have some Brackets code that requires or generates paths in the opposite format. This cleans up several of those cases:
This is an incremental improvement. Not everything has been cleaned up: