Skip to content
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

Updated Claim Interface #226

Merged
merged 10 commits into from
Nov 9, 2018
Merged

Updated Claim Interface #226

merged 10 commits into from
Nov 9, 2018

Conversation

jwicks31
Copy link
Contributor

@jwicks31 jwicks31 commented Nov 8, 2018

PR Process - PR Review Checklist

Release

Semantic release is enabled for this repository. Make sure you follow the right commit message convention.

We're using semantic-release's default — Angular Commit Message Conventions.

Description of Changes

  • Updated interface for Claims

Resolves #227

@jwicks31 jwicks31 self-assigned this Nov 8, 2018
krobi64
krobi64 previously requested changes Nov 8, 2018
Copy link
Contributor

@krobi64 krobi64 left a comment

Choose a reason for hiding this comment

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

PR Review Checklist

  • [na] Tested changes manually
  • Checked accidental architectural/style changes
  • Reviewed entire diff
  • Unit tests
  • [na] Documentation
  • [na] Filenames and locations

PR Reviewer Comments

There are a number of places where you have to update this for unit tests to pass. You should see this in Travis.

@@ -9,12 +9,16 @@ export enum ClaimType {
Work = 'Work',
}

export interface StringToRecursiveObject {
[key: string]: string | number | StringToRecursiveObject
Copy link
Contributor

Choose a reason for hiding this comment

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

This might also need to be an array of items.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which may mean the key can be a number?

Copy link
Member

Choose a reason for hiding this comment

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

I think to support the array we'd do

[key: string]: string | number | StringToRecursiveObject | ReadonlyArray<string | number | StringToRecursiveObject >

Can the array be of any of those types @krobi64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying that the claim itself can be an array?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm saying one of the attributes inside the claim may be an array.

Copy link
Contributor Author

@jwicks31 jwicks31 Nov 8, 2018

Choose a reason for hiding this comment

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

Ok then I think @lautarodragan option should work, assuming that the array can be of numbers, strings, and/or objects?

Copy link
Member

@lautarodragan lautarodragan Nov 8, 2018

Choose a reason for hiding this comment

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

Not the claim, but attributes inside it could (is that it @krobi64?).

I guess something like this would be a valid claim:

{
    name: 'A Study in Scarlet',
    author: 'Arthur Conan Doyle',
    tags: ['detective', 'novel', 'detective'],
    dateCreated: '1886-01-01T00:00:00.000Z',
    datePublished: '1887-01-01T00:00:00.000Z',
    stuff: [ { something: true, somethingElse: 42 } ],
    nestedStuff: [ { reallyComplex: { someNestedProp: 42 } } ],
  },

Copy link
Member

Choose a reason for hiding this comment

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

A simpler alternative would be

export interface StringToRecursiveObject {
  readonly [key: string]: number | string | unknown // maybe add array here too
}

That would allow programmatic access to first-level children of claim as strings or numbers, but no other level, nor mutation.

Copy link
Member

@lautarodragan lautarodragan left a comment

Choose a reason for hiding this comment

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

LGTM

@jwicks31 jwicks31 closed this Nov 9, 2018
@jwicks31 jwicks31 reopened this Nov 9, 2018
@jwicks31 jwicks31 dismissed krobi64’s stale review November 9, 2018 16:32

Changes were made to fix unit tests + handle Array

@jwicks31 jwicks31 merged commit de1a4a3 into master Nov 9, 2018
@jwicks31 jwicks31 deleted the claim-interface branch November 9, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Claim Interface
3 participants