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

feat: move to typescript & tidy up some code #56

Merged
merged 44 commits into from
Aug 18, 2023
Merged

feat: move to typescript & tidy up some code #56

merged 44 commits into from
Aug 18, 2023

Conversation

marksie1988
Copy link
Contributor

@marksie1988 marksie1988 commented Aug 14, 2023

Description

  • refactor!: change yes/no options to boolean true / false
  • refactor!: change no / one / two / three to numbers 0 / 1 / 2 / 3
  • refactor!: remove hidden option from battery.energy instead use 0 to disable
  • refactor: move to Typescript
  • refactor: add defaults for settings to stop duplication in code
  • feat: add translation module for multi language support
  • feat: add support for lux inverter status codes
  • feat: add unit of measurement to energy cost
  • feat: clicking additional loads shows more-info dialog for full and lite cards
  • feat: add daily aux load
  • chore: add issue templates to gather all required information
  • docs: move to sphinx documentation utilising github pages

Breaking changes are noted above with !

Due to the breaking changes the version will update to v2.0.0 as per semver versioning standard

TODO

To merge this change a few things will need doing with the repository settings:

Enable Github Pages:
The docs use sphinx, which publishes to the gh-pages branch and is then hosted on github pages, this needs enabling in the settings of the repo, I think an empty branch called gh-pages may also need creating.

The documentation workflow allows being run manually or will automatically run when a pull request is merged.

Labels
the following labels should be setup and used on each pull request to auto generate the release notes:

  • type/dependencies or dependencies
  • type/language or language
  • type/docs or documentation
  • semver/patch or type/bug or bug
  • semver/minor or type/feat or enhancement
  • semver/major or flag/breaking changes

Automated release

I have sent you a discord message about automated release which i can add if you want to do it but thats up to you based on what i sent in the message

Related issues

N/A

Motivation and Context

By moving to Typescript:

  • Adds error checking to the code which results in cleaner code.
  • Allows the files to be split out into more manageable chunks
  • Allows rollup and babel to squash the code and only bundle required packages which reduces the size

How has this been tested

I have tested this as much as I can with my setup using a lux inverter, I have not been able to test on a sunsynk inverter so additional resting is advised.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • I have updated the version in package.json following semver

@slipx06
Copy link
Owner

slipx06 commented Aug 14, 2023

Hi @marksie1988

Thanks for your interest in this project and your suggestions to make this even better. As you can see I'm not a developer and hacked my way through most of this.
I may need your help to setup the typescript dev environment and figure out some stuff. If you don't mind then I'm keen to move forward.

@marksie1988
Copy link
Contributor Author

Of course, I'm happy to help.

I have one issue to fix that I found after testing my last change but shouldn't be too long until this is ready.

src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@marksie1988 marksie1988 marked this pull request as ready for review August 17, 2023 15:26
@slipx06
Copy link
Owner

slipx06 commented Aug 17, 2023

Can you take a look at show_solar. It throws our an error when set to false.

image

I've sent you a msg on discord about default values when attributes are undefined.

@marksie1988
Copy link
Contributor Author

marksie1988 commented Aug 18, 2023

Can you take a look at show_solar. It throws our an error when set to false.

image

I've sent you a msg on discord about default values when attributes are undefined.

ok this happens because there is an error check that says if someone doesnt define it in their config it will fail. that happens before the defaults are included, but as there is a default the error is redundant so either the default needs removing if you want the error, or leave the default and remove the error (i would do this as it makes it easier for a user to get up and running)

@slipx06
Copy link
Owner

slipx06 commented Aug 18, 2023

Can you take a look at show_solar. It throws our an error when set to false.
image
I've sent you a msg on discord about default values when attributes are undefined.

ok this happens because there is an error check that says if someone doesnt define it in their config it will fail. that happens before the defaults are included, but as there is a default the error is redundant so either the default needs removing if you want the error, or leave the default and remove the error (i would do this as it makes it easier for a user to get up and running)

But the error should not trigger because show_solar attribute is present in the example config I shared. This only happens when show_solar is set to false? If set to true it does not trigger the error.

@marksie1988
Copy link
Contributor Author

Yes that happens because !config.show_solar is essentially saying show_solar === false it would need changing to config.show_solar === 'undefined' then it would only show if it was not set.

However the error is still redundant due to the default being set.

@slipx06 slipx06 added the semver/major A major change label Aug 18, 2023
@slipx06
Copy link
Owner

slipx06 commented Aug 18, 2023

OK I've completed all my testing. I think this is ready for merging. Please can you do one last check with the last commit I made

@marksie1988
Copy link
Contributor Author

That logic shouldn't have needed changing. as you only want it to work if it is set to true

@slipx06
Copy link
Owner

slipx06 commented Aug 18, 2023

That logic shouldn't have needed changing. as you only want it to work if it is set to true

It was messing up the rendering of the additional loads when not set properly and default should be true.

@marksie1988
Copy link
Contributor Author

actually yes you are correct, i was thinking of the show_aux that changed earlier for some reason :)

@marksie1988
Copy link
Contributor Author

Ok i have just updated the workflow to publish the docs.

It should be good to merge, there may be some tidy up we need to do for all the new backend workflows but we can sort that once we know the errors when it runs (if there are any)

@slipx06 slipx06 merged commit 777a907 into slipx06:master Aug 18, 2023
2 checks passed
@marksie1988 marksie1988 deleted the typescript branch August 18, 2023 16:32
@wynand64
Copy link

morning i have a problem i an using soral assistant and pull data in to homeassistant but solar assist is workindg defrend when using power it show negative and when charging shows positive can i switch this uroound in the programming please help

@slipx06
Copy link
Owner

slipx06 commented Aug 28, 2023

@wynand64 you can invert this through a card option. See discussion linked below

#66

@wynand64
Copy link

wynand64 commented Aug 28, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/major A major change type/breaking A major change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants