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

fix: add types to edgeconfig.ts #2699

Merged

Conversation

miettal
Copy link
Contributor

@miettal miettal commented Jul 2, 2024

npx tsc --strict: 3646 -> 3622 errors

@@ -20,7 +20,7 @@
"module": "es2020",
"noImplicitOverride": true,
"lib": [
"es2018",
"es2022",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lib es version is bumped to es2022 to use .flat().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

generally target es version is lower than lib es version to compatibility.
but this project target es is higher than lib es version. ??
anyway, it is not change of target es version, so I think no effect browser compatibility.

public readonly description!: string;
public readonly type!: string;
public readonly isRequired!: boolean;
public readonly isPassword!: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems type and isPassword is mssing so I added these field.

public readonly level: "INFO" | "OK" | "WARNING" | "FAULT";
public readonly persistencePriority: PersistencePriority;
public readonly text: string;
public readonly type!: "BOOLEAN" | "SHORT" | "INTEGER" | "LONG" | "FLOAT" | "DOUBLE" | "STRING";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Non-Null Assertion ! to the mandatory fields.

@@ -413,7 +412,7 @@ export class EdgeConfig {
* Lists all available Factories, grouped by category.
*/
public static listAvailableFactories(factories: { [id: string]: EdgeConfig.Factory }): CategorizedFactories[] {
const allFactories = [
const allFactories: CategorizedFactories[] = [
{
category: { title: 'Simulatoren', icon: 'flask-outline' },
factories: Object.entries(factories)
Copy link
Contributor Author

@miettal miettal Jul 2, 2024

Choose a reason for hiding this comment

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

allFactories.factories was mixing flat list([]) and nested list([[]]). this is not type first. I rewrite flat-style because this is easily expected from the name factories.

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2699      +/-   ##
=============================================
+ Coverage      56.03%   56.04%   +0.01%     
- Complexity      8041     8044       +3     
=============================================
  Files           2059     2059              
  Lines          87513    87513              
  Branches        6418     6418              
=============================================
+ Hits           49032    49037       +5     
+ Misses         36797    36792       -5     
  Partials        1684     1684              

@miettal miettal force-pushed the bugfix/typing-edgeconfig.ts branch 3 times, most recently from 7a27dc4 to 9e68f08 Compare August 1, 2024 01:27
@miettal miettal force-pushed the bugfix/typing-edgeconfig.ts branch from 9e68f08 to 61563e9 Compare August 1, 2024 01:31
@miettal
Copy link
Contributor Author

miettal commented Aug 1, 2024

rebased!

@sfeilmeier sfeilmeier merged commit 2664b13 into OpenEMS:develop Aug 1, 2024
3 of 4 checks passed
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.

2 participants