Export schemas #428

IvanCoronado · 2017-11-16T09:15:55Z

Added "Save model definitions" in the bar menu.
"Save model definitions" options will be enabled just when you have focused a realm browser window.
Languages supported JS and Swift.

closes (partially) #158

…options

*Enable/Disable when a realm browser window gains/loses focus *Export JS *Export swift

* master: Context menu revisited (#426) Browser row caching (#425) Prepare version v1.7.3 Removing the override of default ports (#416) feat (Cell): Show dates in ISO format (#403) feat (realms): Remove timeout while opening realm (#402) # Conflicts: # src/ui/realm-browser/RealmBrowserContainer.tsx

IvanCoronado · 2017-11-16T10:03:57Z

@kraenhansen @bmunkholm The branch has some TS errors related to the schema exporter library. Should I fix this? Or what are we going to do finally with this dependency?

kraenhansen

There's a lot of TS code in the exporter service that I would like to follow the codestyle of the project.

Also yes - I would like the tests to be passing before merging this in.

kraenhansen · 2017-11-16T10:15:52Z

src/ui/realm-browser/RealmBrowserContainer.tsx

@@ -53,6 +54,9 @@ export class RealmBrowserContainer extends RealmLoadingComponent<
      progress: { done: false },
      schemas: [],
    };
+
+    ipcRenderer.on('exportSwiftSchema', this.onExportSwiftSchema);
+    ipcRenderer.on('exportJSSchema', this.onExportJSSchema);


Could we make this a single event name, and use "dash-case", fx: export-schema and have that take a parameter determining to what language the schema should be exported. That would require way less changes to the code as new languages are added.

kraenhansen · 2017-11-16T10:17:37Z

src/ui/realm-browser/RealmBrowserContainer.tsx

+      exp.exportSchema(this.realm);
+      exp.writeFilesToDisk(selectedPaths);
+    });
+  };


Ideally I would move this code into the service and call it by exporter.showSaveDialogAndSave (or something similar).

Isn't it better to keep the exporter functionality without UI so it's easier to migrate to a tool at some point?

That might be a good thing - but it's a very little part of the code, so I'd rather be able to call it from anywhere in the app and have it behave in the same way and build an abstraction around it when we need to.

kraenhansen · 2017-11-16T10:19:06Z

src/services/schema-export/tsconfig.json

+    "forceConsistentCasingInFileNames": true,
+    "outDir": "dist",
+    "allowJs": true,
+    //"declaration": true


This is not valid JSON, just delete the line.

I would actually like if we could delete the whole tsconfig.json here, what is the reason this could not follow the same config as the root project?

kraenhansen · 2017-11-16T10:19:40Z

src/services/schema-export/tslint.json

+      "check-type"
+    ]
+  }
+}


Same with this tslint.json - any reasons we cannot use the main projects linting here?

kraenhansen · 2017-11-16T10:21:31Z

src/services/schema-export/src/schema-exporter.ts

+            const fullpath = fsPath.resolve(path, file.filename)
+            if (debug) {
+                console.log('--- path: ' + fullpath)
+                console.log(file.content)


No console.logs except if it's strictly needed .. alternatively use a logger.

kraenhansen · 2017-11-16T10:22:25Z

src/services/schema-export/package.json

+  "dependencies": {
+    "fs-extra": "^4.0.2"
+  }
+}


I am tempted to say this shouldn't go in .. we might eventually want to release this as its own package, but I think we should leave it out for now. Especially with the generic name and description that it has now.

kraenhansen · 2017-11-16T10:25:06Z

src/main/MainMenu.ts

+      if (ModelDefinitions && ModelDefinitions.submenu) {
+        (ModelDefinitions.submenu as Electron.Menu).items.map(
+          item =>
+            ((item as Electron.MenuItemConstructorOptions).enabled = enable),


Instead of enable vs. disable, I think it should be visible vs hidden.

@kraenhansen I think that the common pattern in desktop app is enables/disables, also in the old app, we are using it like this.

kraenhansen · 2017-11-16T10:28:17Z

src/main/Application.ts

+      window.on('close', () => {
+        this.mainMenu.enableModelDefinitions(false);
+      });
+


Instead of this - I think we should have a way to configure what menus should be used when a specific type of window gets into focus. It could be an extra field returned when calling windowOptions: https://github.com/realm/realm-studio/blob/master/src/windows/WindowType.ts

And then move the on window blur, focus and close event handlers to the WindowManager.ts.

kraenhansen · 2017-11-16T10:29:25Z

src/main/MainMenu.ts

  }

+  public enableModelDefinitions(enable: boolean) {


I dont think its obvious from this name what the function does.

kraenhansen · 2017-11-16T10:30:35Z

src/main/MainMenu.ts

+  ): Electron.MenuItemConstructorOptions =>
+    menu.items.find(
+      item => (item as Electron.MenuItemConstructorOptions).label === name,
+    ) as Electron.MenuItemConstructorOptions;


With this being multiple lines, I would like curly-brackes {} and a return.

bmunkholm · 2017-11-16T11:00:59Z

@IvanCoronado @kraenhansen The code I sent should, by all means, be updated to reflect the style you want. It was just something to get going on the export specific part :-). And my first ever JS/TS code - so I imagine it's quite poor. Refactor and adapt to your liking :-)
Surely needs some cleanup various places.

bmunkholm · 2017-11-16T11:03:27Z

src/services/schema-export/src/schema-exporter.ts

+
+const fs = require('fs-extra');
+
+const debug = true


set to false

IvanCoronado · 2017-11-16T11:09:20Z

@bmunkholm @kraenhansen Yes, I had supposed that we need adapt this code but I wasn't sure if I had to do it myself or someone was working on this task already. I wanted to be sure before to start with this 👍

bmunkholm · 2017-11-16T11:36:25Z

Perfect. Feel absolutely free to adapt it, merge it in right and whatever. Then others can add the missing exports, as that's "trivial code" people from outside can contribute with. Perhaps even myself :-)

IvanCoronado · 2017-11-16T12:10:24Z

@bmunkholm I will fix TS errors and I will update @kraenhansen feedback.

After that, we should add tests before than refactor your code. But I am not sure if I have the knowledge enough about realm and swift for adding the tests coverage.

bmunkholm · 2017-11-16T15:26:26Z

Re. test we should just make a simple one that compares the output of the generated text with that of a file. If you do that I can update the file to compare to. (there are already files in the tests/ directories from OSX Browser. They will have to be fixed up a bit though as what we generate now may differ a bit from the other generated files.).

bmunkholm · 2017-11-17T10:54:00Z

@IvanCoronado If you are cleaning anything up, it would be good to do that quickly, as @nhachicha will help out on specific export for Java (and perhaps the rest)

bmunkholm · 2017-11-17T12:08:32Z

src/services/schema-export/src/schema-exporter/abstractSchemaExporter.ts

+    });
+  }
+
+  public makeSchema(schema: Realm.ObjectSchema) {} // tslint:disable-line


If we are doing this anyway, why have a lint check for it ;-)
Isn't it easier to just have the ending brace on the next line?

This should probably just be an abstract method instead.

Or throw an error saying it must be implemented.

bmunkholm · 2017-11-17T12:09:27Z

src/services/schema-export/src/schema-exporter/index.ts

+  ObjC = 'objC',
+  Swift = 'swift',
+  Java = 'java',
+  CS = 'CS',


bmunkholm · 2017-11-17T12:10:45Z

src/services/schema-export/src/schema-exporter/index.ts

+    case Language.JS:
+      return new JSSchemaExporter();
+    default:
+      return new AbstractSchemaExporter();


Not useful? Throw instead?

…emaOption

…udio into ic/export-schemas

bmunkholm · 2017-11-20T21:52:49Z

@IvanCoronado Others can fix the exporter, or rather, in this case, the models we are comparing against should be updated (as listed in the readme). Those are examples from the OSX browser which has a few bugs in the exporter and other preferences for how to output the model.

@nhachicha can take that, and check up with the Cocoa team if needed.

…-schemas

kraenhansen · 2017-11-21T10:07:30Z

src/services/schema-export/languages/java.ts

+    this.appendLine('package your.package.name.here;');
+    this.appendLine('');
+
+    const realmImports = new Set<string>();


I get an error from the TypeScript compiler here:

TS2495: Type 'Set' is not an array type or a string type.

I would probably just use an array of strings.

const realmImports: string[] = []; realmImports.push('import io.realm.RealmObject;');

Strange I didn't get this error locally.
I need a Set otherwise I'll end up with duplicates since I'm adding imports as I iterate through the properties ...

kraenhansen

As soon as the tests pass, I believe this should get merged in and released.

kraenhansen · 2017-11-21T10:19:22Z

src/services/schema-export/languages/java.ts

+    this.settersContent += `${JavaSchemaExporter.PADDING}public void set${this.capitalizedString(
+      prop.name,
+    )}(${this.javaNameForProperty(prop)} ${prop.name}) {
+  ${JavaSchemaExporter.PADDING}${JavaSchemaExporter.PADDING}this.${prop.name} = ${prop.name};


I think this line gets two spaces two much padding in the export - or the return ☝️ two few.

I'm changing this part to match the expected model in the tests

kraenhansen · 2017-11-21T10:26:21Z

I'm reassigning this to @nhachicha - who will investigate the failing tests and merge this when they pass.

bmunkholm · 2017-11-21T10:43:26Z

src/services/schema-export/tests/index.test.ts

+  before(() => {
+    fs.removeSync(`${TESTS_PATH}/realms`);
+    fs.removeSync(`${TESTS_PATH}/temporal`);
+    sampleRealm = makeRealm(`${TESTS_PATH}/realms/sample/SampleTypes.realm`, [


The Realm is not used for anything, so this can be deleted

it is used in exp.exportSchema(sampleRealm); https://github.com/realm/realm-studio/pull/428/files/c6f962d12f9f4595718979ccc6f9be67515ead88#diff-ea0caba4a04c40a26970f338eb0139c1R36

ahh yeah, sure. I was too fast there :-)

bmunkholm · 2017-11-21T10:47:31Z

src/services/schema-export/tests/index.test.ts

+    ]);
+  });
+
+  it('JS exporter model with sample types', () => {


This code is too repetitive. We should have a generic test testModel(language, model) that will compare a directory of files with the expected output.
We should test the AllTypes model as that includes everything. The Sample model is just useful for development of a new exporter.

…udio into ic/export-schemas

…o ic/export-schemas

I figured that these should have been in the options object - but it doesn't feel practical

I believe mutating the menu items are generally disincuraged

kraenhansen

I made a few changes - anticipating that we soon will be adding more menu items on the different window types.

IvanCoronado added 4 commits November 15, 2017 13:19

feat (MainMenu): Add logic for enable/disable model definitions menu …

13ca7b3

…options

style (MainMenu): Lint

7ebdb1f

feat (MainMenu): Fix ts error

a93543a

Add 'Save model definitions' logic

f246788

*Enable/Disable when a realm browser window gains/loses focus *Export JS *Export swift

IvanCoronado requested a review from kraenhansen November 16, 2017 09:15

IvanCoronado added 2 commits November 16, 2017 10:21

feat (RealmBrowserContainer): Improve electron import

f716f9b

kraenhansen requested changes Nov 16, 2017

View reviewed changes

bmunkholm reviewed Nov 16, 2017

View reviewed changes

src/services/schema-export/src/schema-exporter.ts Outdated

const fs = require('fs-extra');

const debug = true

Copy link

Contributor

bmunkholm Nov 16, 2017

Choose a reason for hiding this comment

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

set to false

kraenhansen mentioned this pull request Nov 17, 2017

Export schema to more languages #435

Open

8 tasks

kraenhansen changed the title ~~Ic/export schemas~~ Export schemas Nov 17, 2017

IvanCoronado added 3 commits November 17, 2017 12:48

feat (schema-export): Remove TS errors and refactor structure

09717ef

feat (RealmBrowserContainer): Unify export schema functions

26258bf

refactor (MainMenu): Call new exportSchema function

c34dafa

bmunkholm reviewed Nov 17, 2017

View reviewed changes

IvanCoronado added 6 commits November 17, 2017 13:38

refactor (MainMenu): Use curly-brackets in getMenuItem

c6d897c

refactor (MainMenu): Rename enableModelDefinitions to enableExportSch…

7323ff5

…emaOption

refactor (schemaExporter): Now is typed as abstract

4521250

refactor (schemaExporter): Now is typed as abstract

c5c1cff

refactor (schema-export): Simplify schema-export file structure

71893e3

feat (schema-export): Add coverage for JS and Swift

5eb427f

nhachicha added 3 commits November 20, 2017 18:52

Merge branch 'ic/export-schemas' of https://github.com/realm/realm-st…

594f0b8

…udio into ic/export-schemas

Update README, merged origin

f75ff86

add missing file

386f65b

IvanCoronado and others added 2 commits November 21, 2017 09:38

refactor (MainMenu): Use exportSchema function for Java

c6f962d

Merge branch 'master' of github.com:realm/realm-studio into ic/export…

c4b2532

…-schemas

kraenhansen reviewed Nov 21, 2017

View reviewed changes

kraenhansen previously approved these changes Nov 21, 2017

View reviewed changes

kraenhansen reviewed Nov 21, 2017

View reviewed changes

kraenhansen assigned nhachicha Nov 21, 2017

bmunkholm reviewed Nov 21, 2017

View reviewed changes

nhachicha mentioned this pull request Nov 22, 2017

Realm schema contains linkingObjects realm/realm-js#1519

Open

kraenhansen mentioned this pull request Nov 22, 2017

Manual write transactions #441

Merged

nhachicha added 3 commits November 23, 2017 11:10

fixing tests

cbef1a7

automatic formatted code

39c7e62

Merge branch 'ic/export-schemas' of https://github.com/realm/realm-st…

0fde821

…udio into ic/export-schemas

nhachicha dismissed kraenhansen’s stale review via 0fde821 November 23, 2017 11:14

Kræn Hansen added 7 commits November 23, 2017 13:08

Merge branch 'ic/export-schemas' of github.com:realm/realm-studio int…

454c520

…o ic/export-schemas

Reverted the eventListenerCallbacks

4b644df

I figured that these should have been in the options object - but it doesn't feel practical

Taking a more functional approach on the menu updating

bafa20f

I believe mutating the menu items are generally disincuraged

Using enum values that are suitable for displaying

b9cbca6

Got a TS error on this line

afee148

Registering the listener on mount + removing it on unmount

7e93481

Adding a message + a defaultPath to the export schema save dialog

516b22e

kraenhansen approved these changes Nov 23, 2017

View reviewed changes

kraenhansen merged commit f52def5 into master Nov 23, 2017

nirinchev deleted the ic/export-schemas branch September 25, 2018 08:44

github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2024

Export schemas #428

Export schemas #428

Conversation

IvanCoronado commented Nov 16, 2017 • edited by bmunkholm

IvanCoronado commented Nov 16, 2017

kraenhansen left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmunkholm commented Nov 16, 2017 • edited

Choose a reason for hiding this comment

IvanCoronado commented Nov 16, 2017

bmunkholm commented Nov 16, 2017

IvanCoronado commented Nov 16, 2017

bmunkholm commented Nov 16, 2017

bmunkholm commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmunkholm commented Nov 20, 2017

kraenhansen Nov 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen left a comment

Choose a reason for hiding this comment

IvanCoronado commented Nov 16, 2017 •

edited by bmunkholm

kraenhansen left a comment •

edited

bmunkholm commented Nov 16, 2017 •

edited

kraenhansen Nov 21, 2017 •

edited