Skip to content
This repository has been archived by the owner on Oct 7, 2021. It is now read-only.

fixing the issue that linked types are always a HalResource #25

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Smolli
Copy link

@Smolli Smolli commented Jan 9, 2018

I was at the point were I was wondering why an embedded HAL resource isn't of the type that it was declared. I debugged and found that little bug. If the HAL name and the TS name don't match, the wrong type is being used. This should fix it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.932% when pulling 16aa36a on Smolli:master into c5b637e on deblockt:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 98.587% when pulling 65594fb on Smolli:master into c5b637e on deblockt:master.

adding lodash for some functional magic
@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage decreased (-1.0%) to 97.945% when pulling ec82cf5 on Smolli:master into c5b637e on deblockt:master.

@Smolli
Copy link
Author

Smolli commented Jan 12, 2018

I added some handling to be able to serialize embedded arrays correctly. And I added lodash if you don't mind. 😃
I will provide the test coverage this weekend.

@Smolli
Copy link
Author

Smolli commented Jan 12, 2018

Could this be a part of the solution for #22?

Copy link
Owner

@deblockt deblockt left a comment

Choose a reason for hiding this comment

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

You must add unit test to show fixed issues

@@ -0,0 +1,6 @@
root = true

[*]
Copy link
Owner

Choose a reason for hiding this comment

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

This file must no be on the project.

Copy link
Author

Choose a reason for hiding this comment

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

I can remove it. But I suggest you think about using EditorConfig as is makes collaborating way more easier. I use it in my projects all the time. And I guess it's some kind of common sense.

Have a look here: http://editorconfig.org/

@@ -18,6 +18,7 @@
"url": "http://github.com/deblockt/hal-rest-client/issues"
},
"dependencies": {
"@types/lodash": "^4.14.92",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this lib very usefull?
I'm trying to have a minimal dependency number.

Copy link
Author

Choose a reason for hiding this comment

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

In my opinion it is. It adds the aspect of functional programming. It makes the code easier to read and to maintain. Since I'm a Java developer, this is something I miss sadly in TypeScript. I understand that you want to keep your footprint as small as possible and I can definitely rewrite the code to be imperative and if you insist, I'll do. But it'll make a tear run down my cheek. ;)

@@ -147,4 +152,23 @@ export class HalResource implements IHalResource {

return json;
}

private serializeProperty(prop, serializer: IJSONSerializer, arrayItem?: any) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is a correct place for this method.
There are a hal-json-serializer file to serialize objects.

Copy link
Author

Choose a reason for hiding this comment

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

This is just an extension to serialize(...) . It is just the part that I tool from serialize(...) to make it callable recursively (lines 143-147). And I added the ability to process arrays. Maybe the name should be something else.

Line 143 of the previous version corresponds to line 161. I used some helper methods of lodash to improve readability.
Old line 144 equals new line 162.
So is old line 146 to new line 169. They are the same.
In between is the ability to read arrays (and the recursive call).

@deblockt
Copy link
Owner

Thanks for this PR. after add unit test, and some code fix.
it can be merged

@deblockt
Copy link
Owner

@Smolli did you have work on your unit test?

@Smolli
Copy link
Author

Smolli commented Jan 22, 2018

Sorry, not right now. They shifted the focus on another project at work. But I try to get my hands on as soon as possible.

Repository owner deleted a comment from partyka1 Dec 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants