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

Moved nightscout code into separated folder #122

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

3timeslazy
Copy link
Contributor

Hi there,

First of all, thank you for your work. This script vastly improved my quality of life.

Secondly, in return I'd like to contribute to this project by adding support for the Nightscout V3 API. Prior to that, I wanted to slightly refactor the code to make it easy to work with two different versions and do not break anything else.

Current PR is the first of two I'm going to raise. Its goal is to prepare the code for the support of two versions without actual support for the new version. It includes:

  • The common interfaces for both APIs
  • The implementation for V1, which is basically the current logic, but behind NightscoutAPI interface
  • Moved environment variables parsing into the readConfig function, to all the variables not be distributed across the index.ts file. For now the function only parses nightscout env, but I'm going to support the remaining ones in the subsequent PR

In the second PR I'm going to:

  • add support for the Nightscout V3 API
  • parse all the variables in the readConfig

I do so to make the review easier for you and me

Thank you in advance!

@timtheis
Copy link

I have discovered that Nightscout already has a built in capability to do this using connect. (https://github.com/nightscout/nightscout-connect) Scroll down towards the bottom for Libre Link Up instructions.

Be sure to look at PR35 for a mod that fixes some problems with the current implementation, but has not been put into production yet. (nightscout/nightscout-connect#35 )

@3timeslazy
Copy link
Contributor Author

3timeslazy commented Jan 28, 2024

@timtheis thank you for you suggestion!

I've tried the nightscout-connect prior to raising this PR and frankly I didn't like it. It's nice to have it as a plugin embedded into the server, but it's not nice when the plugin crashes the whole server.

So, for now, I'd prefer to have it as a separate process running next to the server. I could have contributed to the nightscout itself but it's going to take much longer than patching the script I already know

UPD:
I checked it again and it seems possible to run the connect as a sidecar, so I'm going to give it a try one more time. Thanks!

@timtheis
Copy link

timtheis commented Jan 28, 2024 via email

Copy link
Owner

@timoschlueter timoschlueter left a comment

Choose a reason for hiding this comment

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

Looks good to me! :)

@timoschlueter
Copy link
Owner

@3timeslazy I checked the PR. From my point of view it's ready to be merged. As you seem to have discovered the sidecar solution of nightscout-connect for your purpose, are you still working on implementing v3 based on your interface? :)

@3timeslazy
Copy link
Contributor Author

Hi @timoschlueter

Sorry, I missed your messages. Sure, I’m going to finish my work. I see that the CI is falling, so as soon as I have time, I’ll check what’s falling, fix it and raise the second PR with the V3 implementation

@timoschlueter timoschlueter changed the base branch from main to release/2.5.0 March 26, 2024 09:22
@timoschlueter timoschlueter merged commit b8be1d5 into timoschlueter:release/2.5.0 Mar 26, 2024
1 check failed
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.

3 participants