Basic importRoam #888

anmolarora1 · 2020-10-21T13:55:22Z

fixes #865

Adds some basic functions to parse ROAM JSON into { contextIndexUpdates, thoughtIndexUpdates } that can be sync'd with the state

raineorshine

Functionality looks great! I made some recommendations regarding naming and structure.

In addition, please rename all instances of ROAM to Roam (or roam, as the case may be). Do the same thing for JSON and Json.

raineorshine · 2020-10-21T14:06:34Z

src/@types/ROAM/index.d.ts

+declare module 'roam' {
+  import { GenericObject } from '../../utilTypes'
+
+  interface ROAMChild {


Let's call this RoamBlock since that's what they are called within Roam.

raineorshine · 2020-10-21T14:07:39Z

src/@types/ROAM/index.d.ts

@@ -0,0 +1,19 @@
+declare module 'roam' {
+  import { GenericObject } from '../../utilTypes'


utilTypes was removed and GenericObject was renamed to Index. Not sure how this is compiling.

You're right. I found out that it's being ignored by eslint based on its current configuration. Do you think we should update that?

Yes, thanks!

@raineorshine Do you think we can do this in a different PR or maybe towards the end of this PR because there are other .d.ts files as well that might need to be handled?

Yes, let's do a separate PR. I'd like to know what your overall plan is before starting on it.

raineorshine · 2020-10-21T14:10:22Z

src/test-helpers/exportWithoutRoot.js

+/**
+ * Remove root, de-indent (trim), and append newline to make tests cleaner.
+ */


Unless you are using this function across multiple files, it should be defined in the module it is being used. There's no point adding to the surface area of the internal API if it is only used in one module.

Yes, this is being used by some tests. Moreover, we can use it in other tests where we're using exportContext, to get rid of the ROOT_TOKEN from our tests' assertions, which is quite redundant.

That sounds good. Please replace the duplicate code in other files with a reference to exportWithoutRoot.

Sure. Just to be clear, are you referring to this file, or all the possible use-cases of this function that are currently using exportContext?

Just duplicate functions that can be replaced with an import, like in importHtml, that look like this:

const exportedWithoutRoot = exported.slice(exported.indexOf('\n')) .split('\n') .map(line => line.slice(2)) .join('\n') + '\n'

raineorshine · 2020-10-21T14:11:10Z

src/test-helpers/exportWithoutRoot.js

+/**
+ * Remove root, de-indent (trim), and append newline to make tests cleaner.
+ */
+export const exportedWithoutRoot = exported => exported.slice(exported.indexOf('\n'))


removeRoot would be a better name for this function.

@anmolarora1

raineorshine · 2020-10-21T14:14:18Z

src/util/importROAM.ts

+/**
+ * Parses ROAM JSON and generates { contextIndexUpdates, thoughtIndexUpdates } that can be sync'd to state.
+ */
+export const importROAM = (state: State, thoughtsRanked: SimplePath, ROAM: RoamNode[]) => {


We're calling thoughtsRanked by the name simplePath now for clarity.

raineorshine · 2020-10-21T14:18:46Z

src/util/importROAM.ts

+/**
+ * Converts the ROAM JSON to an array of blocks.
+ */
+const convertROAMJSONToBlocks = (ROAM: RoamNode[]) => {


Let's call this roamJsonToBlocks. The word convert is extraneous. All pure functions "convert` one thing to another.

Good point, thanks 👍

@anmolarora1

Sorry about this miss, @raineorshine . Would refactor it

src/util/__tests__/importRoam.ts

src/@types/ROAM/index.d.ts

raineorshine · 2020-10-21T14:33:42Z

Please name PR titles succinctly. Full descriptions go in the description field.

raineorshine · 2020-10-29T14:37:43Z

src/util/__tests__/importRoam.ts

+  } = importROAM(testState, RANKED_ROOT as SimplePath, testData)
+
+  Object.keys(thoughtIndex)
+    // ignore root blocks of ROAM page since they won't have create/edit time


Let's test that the root blocks have created and lastUpdated set to a new Timestamp.

raineorshine · 2020-10-29T14:39:37Z

src/util/__tests__/importRoam.ts

+
+  Object.keys(thoughtIndex)
+    // ignore root blocks of ROAM page since they won't have create/edit time
+    .filter((_, index) => index !== 0 && index !== 4)


Matching by index here is quite fragile. If the test changes at all it will break, or worse create a false positive. In general we want to avoid creating dependencies between things that are not semantically related, or only incidentally related.

Instead, I would suggest filtering out based on the key.

src/util/__tests__/importRoam.ts

raineorshine · 2020-10-29T14:43:15Z

src/util/addThought.ts

+    created: thoughtOld?.created || created,
+    lastUpdated


raineorshine · 2020-10-29T14:44:51Z

src/util/importJSON.ts

+/** Insert the given value at the context. Modifies contextIndex and thoughtIndex. */
+type insertThought = (value: string, context: Context, rank: number, created?:Timestamp, lastUpdated?: Timestamp) => void


Thanks for eliminating the duplicate code!

Another way to do this is to type insertThought inline and then use typeof insertThought to type the saveThoughts parameter.

raineorshine · 2020-10-29T14:46:38Z

src/util/importROAM.ts

+/**
+ * Converts the ROAM JSON to an array of blocks.
+ */
+const roamJsontoBlocks = (ROAM: RoamPage[]) => {


Please change all instances of ROAM to roam as appropriate. This was requested in my previous review. You should also rename ROAM to Roam in the comments.

raineorshine · 2020-10-29T14:47:01Z

src/util/importROAM.ts

+/**
+ * Recursively converts the roam children to blocks.
+ */
+const convertRoamBlocksToBlocks = (children: RoamBlock[]): Block[] => children.map(({ string, children, 'edit-time': editTime, 'create-time': createTime }) => ({


Please rename as requested in previous review.

raineorshine · 2020-10-29T14:50:05Z

src/util/importROAM.ts

+  return ROAM.map((item: RoamPage) => {
+    return {
+      scope: item.title,
+      children: convertRoamBlocksToBlocks(item.children)
+    }
+  })


I recommend the more succinct inline functions when the function body consists only of a return statement:

return ROAM.map((item: RoamPage) => ({ scope: item.title, children: convertRoamBlocksToBlocks(item.children) }))

Yes, I observed this as well. Updated now 👍

raineorshine · 2020-10-29T20:13:37Z

src/util/__tests__/importRoam.ts

+    }
+  ]
+
+  const roamBlocks = [...testData[0].children, ...testData[1].children]


Let's generalize this to work for any amount of testData:

const roamBlocks = testData.map(roamBlock => roamBlock.children).flat()

raineorshine

Looks good! Just a couple small points.

raineorshine · 2020-11-02T17:57:28Z

src/util/__tests__/importRoam.ts

+  Object.keys(contextIndex)
+    .forEach(contextHash => {
+      expect(contextIndex[contextHash].lastUpdated).toEqual('2020-11-02T01:11:58.869Z')
+    })


If you make assertions in a loop, you also need to assert the expected length of the array. Otherwise if no results came back the test would incorrectly pass.

@anmolarora1 I found an even better way to do this. Using toMatchObject we can assert arbitrary keys in a single assertion. This obviates the need for asserting the number of keys and has the advantage of displaying all values that fail instead of just the first one that fails in the forEach loop. See: https://github.com/cybersemics/em/blob/staged-loading/src/util/__tests__/roamJsonToBlocks.ts#L226-L237

The object can be built up abstractly (i.e. reduce), but here I prefer the hard-coded values as they are easier to read and make it easier to assert exceptions, such as Fruits and Veggies using the edit time of their child.

raineorshine · 2020-11-02T17:58:23Z

src/util/__tests__/importRoam.ts

+      expect(contextIndex[contextHash].lastUpdated).toEqual('2020-11-02T01:11:58.869Z')
+    })
+
+  Object.keys(thoughtIndex)


raineorshine · 2020-11-02T18:04:04Z

src/util/__tests__/importRoam.ts

+    thoughtIndexUpdates: thoughtIndex,
+  } = importRoam(testState, RANKED_ROOT as SimplePath, testData)
+
+  Object.keys(thoughtIndex)


Also assert number of keys here

src/util/importROAM.ts

…ests

… subthoughts

…erties(current timestamp)

raineorshine

I really like how this finished up. Thanks for the thorough testing!

raineorshine · 2020-11-06T14:47:04Z

src/util/validateRoam.ts

+/**
+ * Validates the strucutre of RoamBlocks.
+ */
+const isRoamBlock = (children: RoamBlock[] = []): boolean => children.every(({
+  uid,
+  string,
+  children = [],
+  'create-time': createTime,
+  'create-email': createEmail
+}: RoamBlock) =>
+  Array.isArray(children)
+  && isRoamBlock(children)
+  && typeof uid === 'string'
+  && typeof string === 'string'
+  && typeof createTime === 'number'
+  && typeof createEmail === 'string'
+)


I think it's generally cleaner to extract the mapping logic outside of functions. So isRoamBlock would just take a single RoamBlock. The aggregate can be calculated with children.every(isRoamBlock) in validateRoam. This decouples the validation from the aggregate structure.

raineorshine · 2020-11-06T14:49:40Z

src/util/__tests__/validateRoam.ts

+test('it returns false for an invalid create-time value', () => {
+  const invalidRoamString = JSON.stringify([{ ...testData[0], children: testData[0].children.map(child => ({ ...child, 'create-time': '2020-11-06T08:52:50.742Z' })) }])
+  expect(validateRoam(invalidRoamString)).toBe(false)
+})
+
+test('it returns false for an invalid uid value', () => {
+  const invalidRoamString = JSON.stringify([{ ...testData[0], children: testData[0].children.map(child => ({ ...child, uid: 1011 })) }])
+  expect(validateRoam(invalidRoamString)).toBe(false)
+})
+
+test('it returns false for an invalid string value', () => {
+  const invalidRoamString = JSON.stringify([{ ...testData[0], children: testData[0].children.map(child => ({ ...child, string: 1011 })) }])
+  expect(validateRoam(invalidRoamString)).toBe(false)
+})
+
+test('it returns false for an invalid children value', () => {
+  const invalidRoamString = JSON.stringify([{ ...testData[0], children: 1234 }])
+  expect(validateRoam(invalidRoamString)).toBe(false)
+})


Very thorough! Thanks!

raineorshine · 2020-11-06T14:51:17Z

src/util/__tests__/importText.js

+  //   /**
+  //  * Parses Roam and generates { contextIndexUpdates, thoughtIndexUpdates } that can be sync'd to state.
+  //  */
+  // export const importRoam = (state: State, simplePath: SimplePath, roam: RoamPage[]) => {
+  //   const thoughtsJSON = roamJsonToBlocks(roam)
+  //   return importJSON(state, simplePath, thoughtsJSON, { skipRoot: false })
+  // }


Old code should be removed

anmolarora1 requested a review from raineorshine October 21, 2020 13:55

raineorshine requested changes Oct 21, 2020

View reviewed changes

raineorshine changed the title ~~adds basic functions to import from roam json format and respective t…~~ Basic importRoam Oct 21, 2020

anmolarora1 force-pushed the 865_import_from_roam branch from 9963e3b to df061ef Compare October 29, 2020 12:48

anmolarora1 requested a review from raineorshine October 29, 2020 12:51

raineorshine requested changes Oct 29, 2020

View reviewed changes

raineorshine reviewed Oct 29, 2020

View reviewed changes

anmolarora1 force-pushed the 865_import_from_roam branch 2 times, most recently from dd5e556 to 75882eb Compare November 2, 2020 11:17

anmolarora1 requested a review from raineorshine November 2, 2020 12:08

raineorshine requested changes Nov 2, 2020

View reviewed changes

anmolarora1 added 12 commits November 6, 2020 17:24

adds basic functions to import from roam json format and respective t…

f65dd8a

…ests

adds explicit type conversions

b93a349

minor refactoring

cf72843

adds created and lastUpdated properties

1e9fe57

renames exportWithoutRoot to removeRoot

6e9385f

save create-email and edit-email as '=create-email' and '=edit-email'…

3582af5

… subthoughts

replaces duplicate functions with removeRoot

28d44da

adds tests for contextIndex and defaults for created/lastupdated prop…

5260876

…erties(current timestamp)

renames importRoam to roamJsonToBlocks

20fad6e

adds validateRoam

aba5991

refactors roamJsonToBlocks and respective tests

ddafbfd

binds roamJsonToBlocks and validateRoam to importText

0679694

anmolarora1 force-pushed the 865_import_from_roam branch from 75882eb to 0679694 Compare November 6, 2020 11:54

anmolarora1 requested a review from raineorshine November 6, 2020 11:55

raineorshine approved these changes Nov 6, 2020

View reviewed changes

raineorshine marked this pull request as ready for review November 6, 2020 14:53

raineorshine merged commit efa308b into cybersemics:dev Nov 6, 2020

anmolarora1 pushed a commit to anmolarora1/em that referenced this pull request Mar 10, 2021

Merge pull request cybersemics#888 from anmolarora1/865_import_from_roam

fc84026

		@@ -0,0 +1,19 @@
		declare module 'roam' {
		import { GenericObject } from '../../utilTypes'

		/** Insert the given value at the context. Modifies contextIndex and thoughtIndex. */
		type insertThought = (value: string, context: Context, rank: number, created?:Timestamp, lastUpdated?: Timestamp) => void

Basic importRoam #888

Basic importRoam #888

Conversation

anmolarora1 commented Oct 21, 2020

raineorshine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolarora1 Oct 28, 2020 • 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

raineorshine Oct 31, 2020 • 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

anmolarora1 Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine commented Oct 21, 2020

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

raineorshine 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

raineorshine left a comment • 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

anmolarora1 Oct 28, 2020 •

edited

Loading

raineorshine Oct 31, 2020 •

edited

Loading

anmolarora1 Oct 28, 2020 •

edited

Loading

raineorshine left a comment •

edited

Loading