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 au_vic.js and update source status in au.json #1097

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

majesticio
Copy link
Contributor

This adapter is rate limited by the source, takes ~30 seconds to run.

Copy link
Collaborator

@caparker caparker left a comment

Choose a reason for hiding this comment

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

It works but it could be cleaned/refactored

Lets follow the following rules:

  • Keep things simple and readable
  • declare global variables first and then functions
  • fetchData should return the cb - I know that technically it is because you are passing it to formatData but refer to the first bullet, keep it simple.
  • formatData should only do one thing, take data and reshape it. Ideally I would like to see that formatData takes one row of data and reshapes that one row. And we dont need to use unifyMeasurementUnits that is done later in the measurement.js file
  • Proofread your code once its done and passing all tests. Make sure that its not confusing, you have not left random log.debug lines.

@majesticio
Copy link
Contributor Author

Requested changes have been made for better readability and cleaner logging

Copy link
Collaborator

@caparker caparker left a comment

Choose a reason for hiding this comment

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

Much better, but still some work to do.

  • Lets remove all use of lodash - we dont need it and deepclone is pretty heavy to use in this way.
  • lets not be fetchingmeasurements in the formatdata function. It feels awkward. How about we add something like fetchStationMeasurements, pass the stations to that and then loop and call fetchMeasurements
  • Then you could loop through that response and for each measurement pass the measurement and station info to the formatData method. That way you dont need to use deepclone to prevent you from mutating an object all the time
  • And if you still end up with nested arrays for some reason use [].flat() which is native.

@majesticio
Copy link
Contributor Author

Removed lodash and improved readability

Copy link
Collaborator

@caparker caparker 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. Most sites are not providing much data but there is nothing we can do about that. Seems like most sites only supply particles

@caparker caparker merged commit 8f97677 into main Apr 11, 2024
1 check 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.

None yet

2 participants