Update and streamline method and type names #584

edgarmueller · 2017-06-14T12:10:00Z

Fixes #580:

Also updates test descriptions

coveralls · 2017-06-14T12:14:15Z

Coverage remained the same at 98.399% when pulling 0bbefbd on edgarmueller:topic/namings into 9e7288d on eclipsesource:jsonforms2.

eneufeld

Thank you for the fix, just some small naming annotations

eneufeld · 2017-06-14T13:20:48Z

src/core.ts

 };

 /**
 * Global JSONForms object that holds services and registries.
 */
-export class JsonFormsHolder {
+export class JsonForms {


JsonForms is also not a perfect name. Is this maybe a JsonFormsConfiguration?
But as I don't have a better name for this, I'm fine with it, we just should think about how to name the global variable that is created by webpack

I agree regarding the name of the exported global variable that's created by webpack and I'd probably also name it JsonForms :)
I named it JsonForms because I was hoping that this will be the only globar var that is relevant from end-users perspective when extending JsonForms (e.g. when adding styles or UI schemata), but I have no hard opionion on this.

eneufeld · 2017-06-14T13:25:51Z

src/core/data.service.ts

-  notifyChange(uischema: ControlElement, newValue: any): void {
-    if (uischema !== undefined && uischema !== null) {
-      const pair = getValuePropertyPair(this.data, uischema.scope.$ref);
+  notifyDataChangeListeners(controlElement: ControlElement, newValue: any): void {


In my opinion this name is not correct, as we are calling this on the dataservice in order to notify the dataservice about a datachange, so if a control renderer changes a value it notifies the dataservice.
So a better name would be notifyAboutDataChange ?

Hm, yes, true, I focused too much on the listener aspect here. Actually, we should probably rename and split this up to:

changeData(ControlElement, any) { // assign newValue as before notifyDataChangeListeners() }

What do you think?

I agree, the notifyDataChangeListeners() would be private then?

eneufeld · 2017-06-14T13:27:50Z

src/core/data.service.ts

   * @param {any} newValue the changed data value
   * @param {any} data the current data value
   */
-  notifyChange(uischema: ControlElement, newValue: any, data: any): void;
+  dataChanged(uiSchema: ControlElement, newValue: any, data: any): void;


should we name "uiSchema" -> "controlElement"?

Yes, I missed a couple of those. Will update.

eneufeld · 2017-06-14T13:28:13Z

src/core/data.service.ts

  }

  /**
   * Resolve the ref of the given control against the root data.
   *
-   * @param {ControlElement} uischema
+   * @param {ControlElement} uiSchema


uiSchema -> controlElement

eneufeld · 2017-06-14T13:28:29Z

src/core/data.service.ts

   * @return {any} the de-referenced data chunk
   */
-  getValue(uischema: ControlElement): any {
-    const pair = getValuePropertyPair(this.data, uischema.scope.$ref);
+  getValue(uiSchema: ControlElement): any {


uiSchema -> controlElement

eneufeld · 2017-06-14T13:31:29Z

src/renderers/additional/array-renderer.ts

@@ -41,15 +41,15 @@ export class ArrayControlRenderer extends Renderer implements DataChangeListener
  /**
   * @inheritDoc
   */
-  isRelevantKey (uischema: ControlElement): boolean {
+  needsNotificationAbout (uischema: ControlElement): boolean {


uiSchema -> controlElement

eneufeld · 2017-06-14T13:32:09Z

src/renderers/controls/base.control.ts

    super.disconnectedCallback();
  }

  /**
   * @inheritDoc
   */
-  isRelevantKey (uischema: ControlElement): boolean {
+  needsNotificationAbout (uischema: ControlElement): boolean {


uiSchema -> controlElement

eneufeld · 2017-06-14T13:32:28Z

src/renderers/controls/base.control.ts

@@ -104,7 +104,7 @@ export abstract class BaseControl <T extends HTMLElement>
  /**
   * @inheritDoc
   */
-  notifyChange(uischema: ControlElement, newValue: any, data: any): void {
+  dataChanged(uischema: ControlElement, newValue: any, data: any): void {


uiSchema -> controlElement

eneufeld · 2017-06-14T13:34:17Z

src/renderers/controls/base.control.ts

@@ -161,7 +161,7 @@ export abstract class BaseControl <T extends HTMLElement>
   *
   * @returns {T} the created HTML input element
   */
-  protected abstract get inputElement(): T;
+  protected abstract get createInputElement(): T;


should not be a getter maybe?

eneufeld · 2017-06-14T13:38:10Z

test/json-forms.test.ts

    jsonForms.data = {name: 'foo'};
    const dataSchema = {type: 'object', properties: {name: {type: 'string'}}} as JsonSchema;
    jsonForms.dataSchema = dataSchema;
    jsonForms.connectedCallback();
    t.is(jsonForms.children.length, 0);
 });
-test.cb('jsonforms data and schema', t => {
+test.cb('Connect JSON Forms element asynchronously with data and data schema set', t => {


the async comes from the dataschema resolution, so it is just needed to run the test successfully

I know, but I couldn't think of any other name. Suggestions?

can''t we simply omit the async text in the test? or we make all of them async

coveralls · 2017-06-14T14:57:40Z

Coverage remained the same at 98.399% when pulling 743966c on edgarmueller:topic/namings into 59be224 on eclipsesource:jsonforms2.

* Update and streamline methods names * Update test descriptions

coveralls · 2017-06-19T08:22:59Z

Coverage decreased (-0.001%) to 98.397% when pulling 13aed85 on edgarmueller:topic/namings into 59be224 on eclipsesource:jsonforms2.

eneufeld

Looks good, thank you very much for the naming fix.
There are still some parameters named inconsistent, but this can be fixed in a separate PR if you like

eneufeld · 2017-06-19T09:02:42Z

src/core/data.service.ts

   * @returns whether this listener is interested in the given control element
   */
-  isRelevantKey(uischema: ControlElement): boolean;
+  needsNotificationAbout(uiSchema: ControlElement): boolean;


uiSchema -> controlElement

eneufeld · 2017-06-19T09:04:44Z

src/renderers/additional/array-renderer.ts

  }

  /**
   * @inheritDoc
   */
-  notifyChange(uischema: ControlElement, newValue: any, data: any): void {
+  dataChanged(uischema: ControlElement, newValue: any, data: any): void {


uischema -> controlElement

eneufeld · 2017-06-19T09:05:09Z

src/renderers/additional/table-array.control.ts

@@ -42,15 +42,15 @@ export class TableArrayControlRenderer extends Renderer implements DataChangeLis
  /**
   * @inheritDoc
   */
-  isRelevantKey (uischema: ControlElement): boolean {
+  needsNotificationAbout (uischema: ControlElement): boolean {


uischema -> controlElement

eneufeld · 2017-06-19T09:05:14Z

src/renderers/additional/table-array.control.ts

    return uischema === undefined || uischema === null
    ? false : (<ControlElement>this.uischema).scope.$ref === uischema.scope.$ref;
  }

  /**
   * @inheritDoc
   */
-  notifyChange(uischema: ControlElement, newValue: any, data: any): void {
+  dataChanged(uischema: ControlElement, newValue: any, data: any): void {


uischema -> controlElement

eneufeld · 2017-06-19T09:05:24Z

src/renderers/additional/tree-renderer.ts

@@ -126,12 +126,12 @@ export class TreeMasterDetailRenderer extends Renderer implements DataChangeList
    this.renderFull();
    return this;
  }
-  isRelevantKey (uischema: ControlElement): boolean {
+  needsNotificationAbout (uischema: ControlElement): boolean {


uischema -> controlElement

eneufeld · 2017-06-19T09:05:29Z

src/renderers/additional/tree-renderer.ts

    return uischema === undefined || uischema === null ? false :
      (<ControlElement>this.uischema).scope.$ref === uischema.scope.$ref && !this.addingToRoot;
  }

-  notifyChange(uischema: ControlElement, newValue: any, data: any): void {
+  dataChanged(uischema: ControlElement, newValue: any, data: any): void {


uischema -> controlElement

eneufeld · 2017-06-19T09:05:36Z

src/renderers/additional/tree-renderer.ts

    jsonForms.data = element;
    jsonForms.dataSchema = schema;
    // check needed for tests
    if (jsonForms.addDataChangeListener) {
      jsonForms.addDataChangeListener({
-        isRelevantKey: (uischema: ControlElement): boolean => {
+        needsNotificationAbout: (uischema: ControlElement): boolean => {


uischema -> controlElement

eneufeld · 2017-06-19T09:05:40Z

src/renderers/additional/tree-renderer.ts

          return uischema !== null;
        },
-        notifyChange: (uischema: ControlElement, newValue: any, data: any): void => {
+        dataChanged: (uischema: ControlElement, newValue: any, data: any): void => {


uischema -> controlElement

edgarmueller requested a review from eneufeld June 14, 2017 12:10

edgarmueller added the jsonforms2 label Jun 14, 2017

eneufeld reviewed Jun 14, 2017

View reviewed changes

edgarmueller force-pushed the topic/namings branch 2 times, most recently from e763ba3 to 743966c Compare June 14, 2017 14:53

Fixes eclipsesource#580

13aed85

* Update and streamline methods names * Update test descriptions

edgarmueller force-pushed the topic/namings branch from 743966c to 13aed85 Compare June 19, 2017 08:15

eneufeld approved these changes Jun 19, 2017

View reviewed changes

edgarmueller merged commit 5e6998d into eclipsesource:jsonforms2 Jun 19, 2017

edgarmueller deleted the topic/namings branch March 8, 2018 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update and streamline method and type names #584

Update and streamline method and type names #584

edgarmueller commented Jun 14, 2017

coveralls commented Jun 14, 2017

eneufeld left a comment

eneufeld Jun 14, 2017

edgarmueller Jun 14, 2017 •

edited

Loading

eneufeld Jun 14, 2017

edgarmueller Jun 14, 2017

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

edgarmueller Jun 14, 2017 •

edited

Loading

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

eneufeld Jun 14, 2017

edgarmueller Jun 14, 2017

eneufeld Jun 14, 2017

coveralls commented Jun 14, 2017

coveralls commented Jun 19, 2017

eneufeld left a comment

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

eneufeld Jun 19, 2017

Update and streamline method and type names #584

Update and streamline method and type names #584

Conversation

edgarmueller commented Jun 14, 2017

coveralls commented Jun 14, 2017

eneufeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgarmueller Jun 14, 2017 • edited Loading

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

edgarmueller Jun 14, 2017 • edited Loading

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

coveralls commented Jun 14, 2017

coveralls commented Jun 19, 2017

eneufeld left a 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

edgarmueller Jun 14, 2017 •

edited

Loading

edgarmueller Jun 14, 2017 •

edited

Loading