add basic design for allowing to set letterCase #70

jannishuebl · 2018-01-31T17:42:17Z

This is my suggestion to fix #66 .

What are your opinions?

First to underscore and then to camelcase seems strange to me, but it seems like the semantic of the inflected package is correct that way, since activesupport also has this semantics.

richmolj · 2018-01-31T21:36:12Z

@jannishuebl haven't been able to take a deep look yet but seems like it's coming along nicely. I like letterCase and I'm fine with underscore/camelize - though we should probably extract that same logic into a separate hyphenate function.

richmolj · 2018-02-01T15:50:00Z

After thinking about it a bit more, what do you think about refactoring to something like

// model.ts

serializeKey(key: string) : string {
  if (this.letterCase == 'underscore') {
    // etc
  }
}

deserializeKey(key: string) : string {
  if (this.letterCase == 'underscore') {
    // etc
  }
}

Then in the relevant portions of the code we call serializeKey/deserializeKey. This way we can support easy configuration with letterCase, but users can also override these functions and do whatever type of serialization/deserialization they want. Should also clean up the code a bit.

jannishuebl · 2018-02-01T19:51:26Z

@richmolj I like that too! I'll give it a try.

jannishuebl · 2018-02-01T20:09:59Z

@richmolj would you prefer a static methods for that purpose?

richmolj · 2018-02-01T21:30:05Z

Yeah static makes sense

richmolj

@jannishuebl thanks so much for this work - we're very close to merging along with a 1.0 release. Had some minor comments and then we are good to go!

richmolj · 2018-02-05T18:34:57Z

src/model.ts

@@ -466,7 +468,7 @@ export class JSORMBase {
        let attributeName = key

        if (this.klass.camelizeKeys) {


Since we're allowing backwards-incompatible changes in 1.0, I'd like to remove this and support letterCase === "camelized" the same way you already have letterCase === "dasherized"

In my opinion they both have a different semantic:

letterCase allows to set the format of the api.
camelizeKeys tells jsorm if it should camelize the keys that come from the api or if it should keep the format as it is.

I think this really comes down for naming. Effectively we now have deserializeKey and serializeKey methods - anything dealing with casing should be within these two methods.

So one option would be to move if (this.camelizeKeys) into the serializeKey method's logic. But it doesn't really make sense to have one thing called letterCase and another thing called camelizeKeys.

What about something like static keyFormat = { from: 'dasherized', to: 'camelized' }?

yes putting it to an object with from and to makes totally sense. I think keyFomat is to general what's about keyCase?

keyCase sounds good to me 👍 Please make sure Typescript is only allowing from/to strings we have whitelisted. @wadetandy suggests 'dash' | 'camel' | 'snake'

richmolj · 2018-02-05T18:35:18Z

src/util/deserialize.ts

@@ -241,7 +241,11 @@ class Deserializer {
        let relationName = key

        if (instance.klass.camelizeKeys) {


same note on camelizeKeys

richmolj · 2018-02-05T18:35:32Z

src/util/validation-errors.ts

@@ -44,7 +43,7 @@ export class ValidationErrors {
    let attribute = meta.attribute

    if (this.model.klass.camelizeKeys) {


camelizeKeys

jannishuebl · 2018-02-08T09:36:22Z

test/unit/model-class-typings.test.ts

@@ -44,7 +44,7 @@ describe("Model Class static attributes typings", () => {
          const endpoint: string = RootClass.endpoint
          const jwt: string | undefined = RootClass.jwt
          const jwtLocalStorage: string | false = RootClass.jwtLocalStorage
-          const camelizeKeys: boolean = RootClass.camelizeKeys
+          const keyCase: KeyCase = RootClass.keyCase


I donot know about this test. It should fail in my opinion but does not. also if I change another attributes type to something diffrent.

Since this is my first time writing typescript I appreciate some help here.

Not sure I'm following your question. This test is essentially serving as a smoke test that the type of a particular static property is what you'd expect. If you look at the comment at the top, the test won't actually ever fail, it will just be a type error in your editor or at compilation time. The current state of yarn run test doesn't actually throw type errors, so it's more of a user assistance test than anything.

wadetandy · 2018-02-08T19:32:07Z

src/model.ts

@@ -39,14 +39,19 @@ import { cloneDeep } from "./util/clonedeep"
 import { nonenumerable } from "./util/decorators"
 import { IncludeScopeHash } from "./util/include-directive"

+export interface KeyCase {
+  from: "dash" | "camel" | "snake"


may want to extract these strings to a separate type for easier reuse: type KeyCaseValue = "dash" | "camel" | "snake"

wadetandy · 2018-02-08T19:33:58Z

src/model.ts

@@ -128,7 +133,7 @@ export class JSORMBase {
  static endpoint: string
  static isBaseClass: boolean
  static jwt?: string
-  static camelizeKeys: boolean = true
+  static keyCase: KeyCase = { from: "snake", to: "camel" }


from and to are confusing names for me when the possible arguments are the case themselves. Transitive names of these imply that the arguments are functions, when instead they are 'which case I am in when on a particular side of the server and client interaction'. For this reason I'd suggest renaming to client and either server or api.

wadetandy · 2018-02-08T21:14:56Z

Thanks for making that change. This all looks 🔥 great 🔥

Thanks for the contribution!

jannishuebl added 2 commits January 31, 2018 18:38

add basic design for allowing to set letterCase

da765ec

use underscore to convert to and from camelcase

22bee89

jannishuebl changed the title ~~WIP: add basic design for allowing to set letterCase~~ add basic design for allowing to set letterCase Jan 31, 2018

camalize errors if they are dasherized

38a7a39

add deserializeKey and serializeKey to model

5da4167

make key converters static

da0552e

richmolj reviewed Feb 5, 2018

View reviewed changes

allow to explixitly set keyCase.from and .to

b16f3ef

jannishuebl commented Feb 8, 2018

View reviewed changes

Merge branch '1.x-dev' into allow_to_use_dasherized_keys

0070e40

wadetandy reviewed Feb 8, 2018

View reviewed changes

use server and client instead of from and to

c5420a5

wadetandy merged commit 5af9d3a into jsonapi-suite:1.x-dev Feb 8, 2018

		@@ -466,7 +468,7 @@ export class JSORMBase {
		let attributeName = key

		if (this.klass.camelizeKeys) {

		@@ -241,7 +241,11 @@ class Deserializer {
		let relationName = key

		if (instance.klass.camelizeKeys) {

		@@ -44,7 +43,7 @@ export class ValidationErrors {
		let attribute = meta.attribute

		if (this.model.klass.camelizeKeys) {

add basic design for allowing to set letterCase #70

add basic design for allowing to set letterCase #70

Uh oh!

Conversation

jannishuebl commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richmolj commented Jan 31, 2018

Uh oh!

richmolj commented Feb 1, 2018

Uh oh!

jannishuebl commented Feb 1, 2018

Uh oh!

jannishuebl commented Feb 1, 2018

Uh oh!

richmolj commented Feb 1, 2018

Uh oh!

richmolj 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!

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!

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!

wadetandy commented Feb 8, 2018

Uh oh!

Uh oh!

jannishuebl commented Jan 31, 2018 •

edited

Loading