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

Refactor codebase to TypeScript #22

Merged
merged 10 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@Derpthemeus
Copy link
Member

commented May 10, 2019

No description provided.

@Derpthemeus

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

@PeteBa Would you mind making sure it looks like the local weather feature was merged correctly in 31e7e54 ?

@salbahra salbahra referenced this pull request May 12, 2019

Merged

Finish up pws endpoint #23

@salbahra salbahra merged commit ca4ec4a into OpenSprinkler:master May 13, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@Derpthemeus , so I got a bit sidetracked 8) , and see that this has been merged in. I think it is a great job here. Only question I had was whether we need the filter.pws and filter.gps here as this should be handled by resolveCoordinates() ?

@Derpthemeus

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Good catch, the filter.gps check can indeed be removed (the filter.pws check is still necessary to provide a descriptive error message indicating PWS support was removed). I'll take that out in a bit when I get a chance

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Or for fliter.pws you could change the equivalent error message here in resolveCoordinate() as it would be more applicable.

@Derpthemeus

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Great idea, I'll do that

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

One simplification for server.js but well beyond the scope of this PR 8) is to move the dotenv.config() line to before the declaration of host and port that way you can remove the whole if statement on line 16.

Dotenv default precedence is: environment variable -> .env file -> default value.

This one has bugged me for ages 8)

@salbahra

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Ah good to know! I’ll fix this thanks!

@Derpthemeus Derpthemeus deleted the Derpthemeus:ts-refactor branch May 13, 2019

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@Derpthemeus , OK, so had a chance to take a slower read through weather.ts, three small code simplifications:

Change L134-137:

		let temp = wateringData.temp,
			humidityFactor = ( humidityBase - wateringData.humidity ),
			tempFactor = ( ( temp - tempBase ) * 4 ),
			precipFactor = ( ( precipBase - wateringData.precip ) * 200 );

To:

		let humidityFactor = ( humidityBase - wateringData.humidity ),
			tempFactor = ( ( wateringData.temp - tempBase ) * 4 ),
			precipFactor = ( ( precipBase - wateringData.precip ) * 200 );

L294-295: Stale comments

L297-306: I can't work out this piece of logic. It seems not to relate to the opening comments but if PWS identifiers are re-instated then they should be dealt with in resolveCoordinates() not elsewhere in the code.

Finally, L155, I think a conservative approach would use Math.ceil() given that this is about watering plants and a bit more is better than a bit less. 8)

Anyway, I had some other thoughts but there more around refactoring so let me know if your interested but right now it's past midnight in London. Again, great work,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.