feat(actualV1): Actual Launch! Symbolic v1! Let's GOOOO! by DanielSeehausen · Pull Request #19 · ospin-web-dev/FCTGraph

DanielSeehausen · 2021-06-10T16:51:11Z

BREAKING CHANGE: breaks the fake major version!

Ignore the diff! please review the code from /src.

I can best recommend reading the README since that attempts to outline the major architecture and will make looking at the code a lot more straight forward I believe.

My goal while writing this was EXtenSIbiliTy so I would particularly appreciate any feedback regarding that. I wanted it to be open for UNCLE extension and closed for BOB modification. E.g.

easy to add new complex fcts by following the pattern provided
hard to circumvent the SCHEMA structures
easy to test
hard to mutate anything that would fail data validation
easy to get to/from JSON.

closes ospin-web-dev/REACTor#1329

* moves seeder from test to src directory * exports the seeders and the datastream from the package index.js Co-authored-by: Viktor Diezel <viktor.diezel@ospin.de>

vdiezel · 2021-06-11T06:43:03Z

    }
  }

+  static get UNITLESS_UNIT() { return Slot.UNIT_TYPE_UNIT_OPTIONS.unitless[0] }


Here you are implicitly using the "magic string" unitless. A safer options would be

Slot.UNIT_TYPE_UNIT_OPTIONS[Slot.UNIT_TYPES.UNITLESS][0]

Or write a helper method if that is too ugly:

static getUnitOptionsFromUnitType(type) { return Slot.UNIT_TYPE_UNIT_OPTIONS[type] } const unitLessUnit = Slot.getUnitOptionsFromUnitType(Slot.UNIT_TYPES.UNITLESS)[0]

v good 👁️ ! updated

vdiezel · 2021-06-11T06:51:51Z

+    if (
+      this.unit !== otherSlot.unit
+      && !this.isUnitless()
+      && !otherSlot.isUnitless()
+    ) {
      throw new SlotConnectionError(this, otherSlot, 'units must match between slots')
    }
+  }


if I have one slot that is unitless and another one that is not, should they be compatible? If I'm not mistaken, this code says yes. Do we want that behaviour or should we treat unitless just as any other "unit" by saying: only unitless slots can be connected.

I see that we might be be missing the difference between: Allow any unit which might be required for certain customer made functionalities, like an "averager" functionality, and functionalities that allow only unitless input, like PID parameters.
Your solution covers the "averager" case: We don't know which units are going in, so we can't create the slots accordingly with the correct unit. It would also allow putting in non-sense values e.g. to the PID slots.
Maybe we should make a difference between unitless as - and null ? Or introduce a new string identifier, like any.

screw that, total freedom!

vdiezel

very professional - very easy to read and the tests show how easy the interface is to use.
I have nothing to complain about - I think my only chance to contribute would be only after actually using it for a while.

codecov · 2021-06-11T12:51:47Z

Codecov Report

Merging #19 (d284171) into main (26a3a16) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #19   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        27    +2     
  Lines          458       489   +31     
  Branches        32        30    -2     
=========================================
+ Hits           458       489   +31

Impacted Files	Coverage Δ
src/slots/InSlot.js	`100.00% <ø> (ø)`
src/slots/SlotConnectionError.js	`100.00% <ø> (ø)`
src/dataStreams/DataStream.js	`100.00% <100.00%> (ø)`
src/fctGraph/AddFunctionalityError.js	`100.00% <100.00%> (ø)`
src/fctGraph/FCTGraph.js	`100.00% <100.00%> (ø)`
src/functionalities/Functionality.js	`100.00% <100.00%> (ø)`
src/functionalities/index.js	`100.00% <100.00%> (ø)`
src/mixins/instanceMixins/JOIous.js	`100.00% <100.00%> (ø)`
src/slots/OutSlot.js	`100.00% <100.00%> (ø)`
src/slots/Slot.js	`100.00% <100.00%> (ø)`
... and 9 more

DanielSeehausen · 2021-06-15T09:56:21Z

-    this.slots = Array.isArray(slots)
-      ? slots.map(SlotFactory.new)
-      : []
+    this.slots = []


I recall a best practice when initializing objects is to always provide everything the object will need in its lifecycle in the constructor (e.g., avoid doing things like adding a new this.property in some helper method because that makes the instance a lot less transparent). For this reason are empty slots added here, then immediately populated in the add slots call.

DanielSeehausen · 2021-06-15T09:57:20Z


  static deepEquals(objA, objB) {
-    const objDiff = this.diff(objA, objB)
+    const objDiff = this.diff(objA.serialize(), objB.serialize())


only the serialized results count as equality. Anything not communicated in serialization is internal to the fctgraph workings, and not persisted when it is sent/stored as JSON

DanielSeehausen · 2021-06-15T09:58:32Z

+  static generateSlots(functionalityId) {
+    if (!Array.isArray(this.SLOT_SEEDS)) {
+      throw new Error(`${this.name} must have .SLOT_SEEDS to use .generateSlots`)
+    }
+
+    return this.SLOT_SEEDS.map(slotData => ({
+      ...slotData,
+      functionalityId,
+    }))
+  }
+


this is used now in all children of FunctionalitySeeder (e.g. TemperatureSensor) because they must send their functionality id to their slots!

DanielSeehausen · 2021-06-15T10:02:59Z

+    const fctData = {
+      ...super.generate(overrideData),
      subType: HeaterActuator.SUB_TYPE,
-      slots: [ ...this.SLOT_SEEDS ],
-      ...data,
+    }


first, do what is required to generate an id

DanielSeehausen · 2021-06-15T10:03:53Z

+      ...fctData,
+      slots: [ ...this.generateSlots(overrideData.id || fctData.id) ],
+      ...overrideData,


then use the id when making the slots (or use the provided id)

(the changes to this FCT seeder is recreated in all of the usable FCT seeders)

DanielSeehausen · 2021-06-15T10:05:00Z

  }

-  static get UNITLESS_UNIT() { return Slot.UNIT_TYPE_UNIT_OPTIONS.unitless[0] }
+  static get UNITLESS_UNIT() { return Slot.UNIT_TYPE_UNIT_OPTIONS[Slot.UNIT_TYPES.UNITLESS][0] }


requested change

DanielSeehausen · 2021-06-15T10:05:51Z

ready for review!
fa13444
this is the only commit that needs review ^

….com/ospin-web-dev/FCTGraph into finishing-touches-for-initial-deploy

FelixMZ2018

Looks great! The tests make it really easy to understand usage and concepts!
I left a few comments but nothing is blocking from my side

FelixMZ2018 · 2021-06-15T13:39:40Z

-[![codecov](https://codecov.io/gh/ospin-web-dev/FCTGraph/branch/master/graph/badge.svg)](https://codecov.io/gh/ospin-web-dev/FCTGraph)
-[![Maintainability](https://api.codeclimate.com/v1/badges/ab083cc74a1fbb1d7319/maintainability)](https://codeclimate.com/repos/60ae147b04beeb018b015a77/maintainability)
+[![codecov](https://codecov.io/gh/ospin-web-dev/FCTGraph/branch/main/graph/badge.svg?token=RXXLX0HDAR)](https://codecov.io/gh/ospin-web-dev/FCTGraph)
+[![Maintainability](https://api.codeclimate.com/v1/badges/184840b3c795f19f837b/maintainability)](https://codeclimate.com/github/ospin-web-dev/FCTGraph/maintainability)


Should codeclimate be removed here ?

good question - no because this is public (which codeclimate works on)

FelixMZ2018 · 2021-06-15T16:43:27Z

    "test-release": "npx semantic-release --dry-run",
    "test": "jest",
-    "test-with-coverage": "jest -i --coverage",
+    "test-with-coverage": "jest --coverage",


Probably worth changing the script title in hambda so all 3 projects use the same syntax

FelixMZ2018 · 2021-06-15T16:45:10Z

@@ -24,7 +24,7 @@
    "start": "node index.js",
    "test-release": "npx semantic-release --dry-run",


I dont think that would work because it can only be called from the master branch

this guy provides(ed) valuable information for testing releases!

FelixMZ2018 · 2021-06-16T07:40:33Z

+    return util.inspect(this, { compact: false, depth: 4 })
  }

  // deeper print outs while running on Node


Should the deeper printout have a higher depth than 4 ?

good question and 👁️ ! - I actually chose 4 off the cuff assuming it would get us all the way down to data streams from an FCTGraph printout and no further. Turns out it doesn't (but 6 does!).

{ fctGraph: { fcts: { slots: { dataStreams } }

I have updated it (and also fixed unfound mistake in which datastreams weren't being serialized).

Co-authored-by: Felix Menzel <36296555+FelixMZ2018@users.noreply.github.com>

github-actions · 2021-06-16T10:59:12Z

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

DanielSeehausen and others added 7 commits June 10, 2021 18:35

adds essential functionalty; adds test coverage; coverage at %98.77

0044d54

finished with real v1

be6f02b

minor fixes - start of documentation

6d86c66

update readme

6291e06

Export seeder (#18)

f7aeeaa

* moves seeder from test to src directory * exports the seeders and the datastream from the package index.js Co-authored-by: Viktor Diezel <viktor.diezel@ospin.de>

add seeders to ignore paths

714d424

updates readme

b6eb4f4

DanielSeehausen requested review from FelixMZ2018 and vdiezel June 10, 2021 16:51

DanielSeehausen self-assigned this Jun 10, 2021

update workflow names

fbb7997

DanielSeehausen force-pushed the finishing-touches-for-initial-deploy branch from 31ad621 to fbb7997 Compare June 10, 2021 17:08

DanielSeehausen added 2 commits June 10, 2021 19:13

trigger cicd

ad78ce1

update codeclimate badge

dfcbcd2

vdiezel reviewed Jun 11, 2021

View reviewed changes

vdiezel previously approved these changes Jun 11, 2021

View reviewed changes

DanielSeehausen dismissed vdiezel’s stale review via 4abed6d June 11, 2021 12:40

DanielSeehausen force-pushed the finishing-touches-for-initial-deploy branch from 4abed6d to b6fbb8f Compare June 11, 2021 12:42

DanielSeehausen force-pushed the finishing-touches-for-initial-deploy branch 6 times, most recently from e7db921 to 02bb9c8 Compare June 15, 2021 09:53

DanielSeehausen commented Jun 15, 2021

View reviewed changes

updates cicd; updates schemas; updates fct refs

fa13444

DanielSeehausen force-pushed the finishing-touches-for-initial-deploy branch from 02bb9c8 to fa13444 Compare June 15, 2021 10:02

DanielSeehausen commented Jun 15, 2021

View reviewed changes

DanielSeehausen and others added 4 commits June 15, 2021 16:33

remove slot reference to parent fctid from serialized results

d7ce80e

bump setup node version

61829b3

Changed incompatible slot type tests to use Kelvin and Celsius

1eb640e

Merge branch 'finishing-touches-for-initial-deploy' of https://github…

89ac861

….com/ospin-web-dev/FCTGraph into finishing-touches-for-initial-deploy

FelixMZ2018 previously approved these changes Jun 16, 2021

View reviewed changes

adds fct stubs where required

b714781

DanielSeehausen dismissed FelixMZ2018’s stale review via b714781 June 16, 2021 09:28

DanielSeehausen force-pushed the finishing-touches-for-initial-deploy branch from dfa83ad to b714781 Compare June 16, 2021 09:28

Update test/mixins/instanceMixins/JOIous.test.js

d284171

Co-authored-by: Felix Menzel <36296555+FelixMZ2018@users.noreply.github.com>

DanielSeehausen force-pushed the finishing-touches-for-initial-deploy branch from 5945b3b to d284171 Compare June 16, 2021 10:23

vdiezel approved these changes Jun 16, 2021

View reviewed changes

DanielSeehausen merged commit 9ac6788 into main Jun 16, 2021

DanielSeehausen deleted the finishing-touches-for-initial-deploy branch June 16, 2021 10:58

github-actions Bot added the released label Jun 16, 2021

		@@ -24,7 +24,7 @@
		"start": "node index.js",
		"test-release": "npx semantic-release --dry-run",

Conversation

DanielSeehausen commented Jun 10, 2021

Ignore the diff! please review the code from /src.

Uh oh!

vdiezel Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdiezel Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdiezel left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielSeehausen Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielSeehausen commented Jun 15, 2021

Uh oh!

FelixMZ2018 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vdiezel Jun 11, 2021 •

edited

Loading

vdiezel Jun 11, 2021 •

edited

Loading

codecov Bot commented Jun 11, 2021 •

edited

Loading

DanielSeehausen Jun 15, 2021 •

edited

Loading