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

Fixed the problem with non numbers primary columns #3

Merged
merged 3 commits into from
Aug 23, 2020

Conversation

gelito
Copy link

@gelito gelito commented Aug 22, 2020

Currently, if you use a different PrimaryColumn with strings (perhaps using the 'uuid' method) the application fails.

The problem is because it's trying to move from string to number without taking into consideration if it's a number o string.

I fixed and refactor the method, trying to do it less complex.
I changed also the test/specs to check with uuid PrimaryGeneratedColumn('uuid')

Copy link

@wojtek-krysiak wojtek-krysiak left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good, I spotted a couple of things which could break ts. Tomorrow I will try to test it.

I will also change target to beta branch so we can deploy this there first.

spec/Resource.spec.ts Outdated Show resolved Hide resolved
spec/Resource.spec.ts Outdated Show resolved Hide resolved
spec/entities/CarDealer.ts Outdated Show resolved Hide resolved

if (!type) { console.warn(`Unhandled type: ${this.column.type}`) }

return type || 'string'
return type;

Choose a reason for hiding this comment

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

I am just thinking if this won't break type checking for strict mode in tsconfig - will have to check it out.

@@ -41,8 +41,8 @@ export class Resource extends BaseResource {
return [...Object.values(this.propsObject)]
}

public property(path: string): Property | null {
return this.propsObject[path] || null

Choose a reason for hiding this comment

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

same here - property should return either BaseProperty or null, When you remove || null it could return undefined and this could break types.

Choose a reason for hiding this comment

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

but I will have to check it out

Copy link
Author

@gelito gelito Aug 22, 2020

Choose a reason for hiding this comment

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

In theory, all works fine with undefined and reduce the code complexity, but we can rollback it, for sure!

src/Resource.ts Show resolved Hide resolved
@wojtek-krysiak wojtek-krysiak changed the base branch from master to beta August 23, 2020 10:37
@wojtek-krysiak wojtek-krysiak merged commit 17a02fe into SoftwareBrothers:beta Aug 23, 2020
@github-actions
Copy link

🎉 This PR is included in version 1.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 1.2.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Apr 3, 2021

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants