-
Notifications
You must be signed in to change notification settings - Fork 324
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
Project Sharing #6077
Project Sharing #6077
Conversation
# Conflicts: # app/ide-desktop/lib/client/src/config/parser.ts # app/ide-desktop/lib/client/src/index.ts # app/ide-desktop/lib/client/src/paths.ts
// We parse only "client arguments", so we don't have to worry about the Electron-Dev vs | ||
// Electron-Proper distinction. | ||
let openedFile = attemptingToOpenFile(paths.clientArguments) | ||
// If we are opening a file (i.e. we weere spawned with just a path of the file to open as |
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.
weere
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.
Fixed.
|
||
/** Update project's metadata. If the provided updater does not return anything, the metadata file is left intact. | ||
* | ||
* The updater function-returned metadata is passed over. |
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 don't understand this line, could you elaborate?
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.
Fixed.
CHANGELOG.md
Outdated
@@ -126,6 +126,7 @@ | |||
shortened labels for entries with long module paths. When an option is | |||
selected from the dropdown, the necessary module imports are inserted, | |||
eliminating the need for fully qualified names. | |||
- [File associations on Windows and macOS][6077] |
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.
- Make it a sentence (dot missing).
- Please add here a little bit more words as this is user facing - it should explain in nice words what it enables :)
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.
Fixed.
app/ide-desktop/lib/client/paths.ts
Outdated
export const SOURCE_FILE_EXTENSION = '.enso' | ||
|
||
export const BUNDLED_PROJECT_EXTENSION = '.enso-project' |
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 don't think this is a good place to keep these constants. As described, "This module defines paths within the client distribution's resources.". I'd keep these in a separate module dedicated to opening URLs functionality.
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 will move this to the other project management stuff.
console.log(`Chrome options: ${chromeOptions}`) | ||
console.log(`Arguments after stripping Chrome options: ${argv}`) |
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.
debug logs left in the code
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.
Fixed.
extension === `.${paths.BUNDLED_PROJECT_EXTENSION}` || | ||
extension === `.${paths.SOURCE_FILE_EXTENSION}` |
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.
this should be a helper function.
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.
Done.
extension === `.${paths.SOURCE_FILE_EXTENSION}` | ||
) { | ||
// Not the file extension we care about, let the default handler handle it. | ||
return |
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.
- we are trying not to use early exists, couldd you just use
if
without return, please? - I don't understand it. You are checking the extensions and if they are our target extensions we are not handling them? Is the if condition correct? If it is, I need some explanation in the code here.
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.
Good catch, the check must've got inverted when refactoring.
// Otherwise, we need to install it first. | ||
if (rootPath === null) { | ||
// Perhaps bundle? | ||
let message = `File '${openedPath}' does not belong to the Enso project.` |
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.
hardcoded product name
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.
Fixed.
export function getCommonPrefix(a: string, b: string): string { | ||
let i = 0 | ||
while (i < a.length && i < b.length && a[i] === b[i]) { | ||
i++ | ||
} | ||
return a.slice(0, i) | ||
} |
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.
this should be extracted to some string utils file
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.
Moved.
/** If the bundle consists of a single directory, return its name. Otherwise, return null. */ | ||
export function bundleRootDirectory(bundlePath: string): string | null { |
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.
Function named bundleRootDirectory
returns directory name and it is not bundling the files? Either name of this fn or docs are wrong and it makes it confusing to me.
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've changed the name.
return updatedMetadata | ||
} | ||
|
||
/** Get the project root path from its subtree. */ |
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.
Could you add here a sentence or two with better explanation what it means, please?
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.
Fixed.
return crypto.randomUUID() | ||
} | ||
|
||
export function updateId(projectRoot: string): 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.
no docs - please check why linter did not catch it, it should
Btw, I adore how well the code is commented. Amazing job with it ❤️ |
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.
My two cents. Sorry for late review.
/// The project name we want to open on startup. | ||
pub project_name: Option<ProjectName>, | ||
pub project_to_open: Option<ProjectToOpen>, |
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.
Not sure if you realized (the docs here weren't helpful TBH), but the option not always is "project to open" - in cloud (i.e. when we receive LS endpoints instead of PM endpoint) this is a name of project we connect to - so not project to open (as it is, technically, already opened), but project name.
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.
As discussed on the call, no change needed here,
* | ||
* @returns The path to the file to open, or `null` if no file was specified. | ||
**/ | ||
export function attemptingToOpenFile(clientArgs: string[]): string | null { |
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.
Why this function is called attempting instead of attempt or try? We were never using -ing endings in fn names.
Actually, this function does not attempt, only checks if the options say we are attempting, so I'd say the name is good.
@@ -74,6 +140,44 @@ class App { | |||
} | |||
} | |||
|
|||
processArguments() { |
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 think this function name is misleading. It does not only processes arguments, but also tries to open the file.
As my understanding of attemptingToOpenFile
goes (see above), the processArguments
name is good. The openedFile
is the misleading name, actually.
@@ -0,0 +1,237 @@ | |||
/** @file This module contains functions for importing projects into the Project Manager. |
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.
we already have
src/bin/project-manager
. Having alsosrc/projectManagement
(btw, wring name style), is confusing. Can we somehow merge them or rename this file?
I'd recommend not merging, as there is nice responsibility separation (Project Mana-ger bindings vs Project Mana-gement)
…aring # Conflicts: # CHANGELOG.md # app/ide-desktop/package-lock.json
|
||
export * from '../file-associations' | ||
|
||
/* Check if the given path looks like a file that we can open. */ |
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.
no sections
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.
Fixed.
@@ -0,0 +1,65 @@ | |||
import pathModule from 'node:path' |
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.
no module docs - linter should catch it. I've written about it in my previous code review - could you please check linter config and check why it did not report here?
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.
@wdanilo I failed to find where the linter is invoked. I tried running ./run lint
, and it found no issues, though I don't see it linted anything. How is the TS linting supposed to work now?
return handleOpenFile(path) | ||
} else { | ||
// We need to start another copy of the application, as the first one is already running. | ||
logger.log(`We are already initialized, opening '${path}' in a new instance.`) |
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.
Very minot We are ...
- The app is ...
- this is just a little bit more formal.
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.
Fixed.
* | ||
* @returns The path to the file to open, or `null` if no file was specified. | ||
**/ | ||
export function attemptingToOpenFile(clientArgs: string[]): string | null { |
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.
- Oh, then I'd still rename it to something like
isDuringFileOpenAttempt
. - TBH, I'd not understand this from the current docs
Check if the given arguments show that we've been invoked with a file to open.
, especially the partwe've been invoked with a file to open
does not tell me that this function checks whether we are attempting currently to open a file. IT rather suggests me that this function was called with a file to open argument.
* @returns The path to the file to open, or `null` if no file was specified. | ||
**/ | ||
export function attemptingToOpenFile(clientArgs: string[]): string | null { | ||
// If we are invoked with exactly one argument and this argument is a file, we assume that we have been |
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.
"if we" -> "if the app" , minor
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.
Fixed.
// project containing this file. | ||
if (clientArgs.length === 1 && clientArgs[0] !== undefined) { | ||
try { | ||
fsSync.accessSync(clientArgs[0]) |
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.
if you extract const arg = cleintArgs[0]
, then several next lines will be simplified.
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.
Fixed.
import * as path from 'node:path' | ||
|
||
import * as electron from 'electron' | ||
import electronIsDev from 'electron-is-dev' | ||
|
||
import * as paths from '../paths' | ||
import * as paths from '../paths.js' |
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.
Why this change? I think we do not have any JS sources anymore here, right? So no extension should work.
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.
Not needed anymore. Actually having or not JS files doesn't make a difference, you need to import TS with js
extension.
The extension is needed when running this through ts-node
, esbuild
seems to not care.
/** Decide what are client arguments, @see {@link clientArguments}. */ | ||
function getClientArguments(): string[] { | ||
if (electronIsDev) { | ||
// Client arguments are separated from the electron dev mode arguments by a '--' argument. | ||
const separator = '--' | ||
const separatorIndex = process.argv.indexOf(separator) | ||
if (separatorIndex === -1) { | ||
// If there is no separator, client gets no arguments. | ||
return [] | ||
} else { | ||
// Drop everything before the separator. | ||
return process.argv.slice(separatorIndex + 1) | ||
} | ||
} else { | ||
// Drop the leading executable name. | ||
return process.argv.slice(1) | ||
} |
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 don't think this function suits this module, as it contains "File system paths used by the application.".
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.
Agreed, moved this one.
// ======================= | ||
// === Project Import === |
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.
lines not aligned (too many =
in first and third line).
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.
Fixed.
…aring # Conflicts: # CHANGELOG.md
…aring # Conflicts: # CHANGELOG.md
* develop: Project Sharing (#6077) Adjust `{Table|Column}.parse` to use `Value_Type` (#6213) Add cloud endpoints for frontend (#6002) Implement `Table.union` for Database backend (#6204) Batch insert suggestions (#6189) Formatter fix to not fail when encountering an invalid symlink. (#6172) Suspended atom fields are evaluated only once (#6151) Text.to_display_text is (shortened) identity (#6174) Engine benchmark visualization tool can compare two bench runs (#6198) Add PRIVATE so function hidden from Component Browser and other tidying... (#6207) Hotfix for #6203. (#6210)
Enso will now associate with two file extensions: * `.enso` — Enso source file. * If the source file belongs to a project under the Project Manager-managed directory, it will be opened. * If the source file belongs to a project located elsewhere, it will be imported into the PM-managed directory and opened; * Otherwise, opening the `.enseo` file will fail. (e.g., loose source file without any project) * `.enso-project` — Enso project bundle, i.e., `tar.gz` archive containing a compressed Enso project directory. * it will be imported under the PM-managed directory; a unique directory name shall be generated if needed. ### Important Notes On Windows, the NSIS installer is expected to handle the file associations. On macOS, the file associations are expected to be set up after the first time Enso is started, On Linux, the file associations are not supported yet.
Pull Request Description
Enso will now associate with two file extensions:
.enso
— Enso source file..enseo
file will fail. (e.g, loose source file without any project).enso-project
— Enso project bundle, i.e.tar.gz
archive containing a compressed Enso project directory.Important Notes
On Windows the NSIS installer is expected to handle the file associations.
On macOS the file associations are expected to be set up after the first time Enso is started,
On Linux, the file associations are not supported yet.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.