Skip to content

Convert process-config-entities to TypeScript#2113

Merged
balloob merged 5 commits intohome-assistant:devfrom
iantrich:ts-process
Dec 4, 2018
Merged

Convert process-config-entities to TypeScript#2113
balloob merged 5 commits intohome-assistant:devfrom
iantrich:ts-process

Conversation

@iantrich
Copy link
Copy Markdown
Member

No description provided.

@ghost ghost assigned iantrich Nov 26, 2018
@ghost ghost added the in progress label Nov 26, 2018
import { EntityConfig } from "../entity-rows/types";

export default function processConfigEntities(entities) {
export const processConfigEntities = (entities: EntityConfig[]) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not correct. The output is EntityConfig[], the input can also be a string (as tested for on line 19) so should be Array<EntityConfig | string>. Also please mark the return type

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that it should be something like this:

Suggested change
export const processConfigEntities = (entities: EntityConfig[]) => {
export const processConfigEntities = <T extends EntityConfig>(entities: Array<EntityConfig | string>[]): T[] => {

public setConfig(config: Config): void {
this._config = { theme: "default", ...config };
const entities = processConfigEntities(config.entities);
const entities = processConfigEntities(config.entities) as ConfigEntity[];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should have processConfigEntities take a type that you expect return values to be.

(also, ConfigEntity and EntityConfig, we must really hate our future selves picking such confusing names)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const entities = processConfigEntities(config.entities) as ConfigEntity[];
const entities = processConfigEntities<ConfigEntity>(config.entities);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, reworking types for cards is on my todo list

@iantrich
Copy link
Copy Markdown
Member Author

I'm not sure how to resolve the TypeScript issues that are now present after making the suggested changes:
image

@iantrich
Copy link
Copy Markdown
Member Author

iantrich commented Dec 3, 2018

@balloob do you have any guidance on getting past my errors, by chance? Thanks.

@balloob
Copy link
Copy Markdown
Member

balloob commented Dec 3, 2018

Doesn't this work?

return entities.map((entityConf, index): T[] => {

Still a typing error which seems like it shouldn't exist
@iantrich
Copy link
Copy Markdown
Member Author

iantrich commented Dec 4, 2018

I believe this is related: palantir/tslint#4239

@balloob balloob merged commit 5dc0512 into home-assistant:dev Dec 4, 2018
@ghost ghost removed the in progress label Dec 4, 2018
@balloob balloob mentioned this pull request Dec 5, 2018
@iantrich iantrich deleted the ts-process branch December 11, 2018 23:03
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants