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

Datafarm #6

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

Datafarm #6

wants to merge 35 commits into from

Conversation

wkrill
Copy link

@wkrill wkrill commented Mar 29, 2023

Draft

@ecomodeller ecomodeller marked this pull request as draft March 30, 2023 05:13
@wkrill wkrill requested a review from jsmariegaard May 8, 2023 11:12
zipped = zip(
o["schema"]["fields"], pj.build_table_schema(df, index=False)["fields"]
) # ignoring index from the newly created frame
for m, l in zipped:
Copy link
Member

Choose a reason for hiding this comment

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

Please use slightly longer names (or at least avoid l as variable name) in for m, l in zipped

response.raise_for_status()
self.access_token = response.headers["Access-Token"]
self.headers = {"Access-Token": self.access_token}
return self.access_token
Copy link
Member

Choose a reason for hiding this comment

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

Why do you return the access_token? Is that meaningful?

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right, it is better to leave it out.


return to_pandas_df(json.dumps(data))

def get_data(
Copy link
Member

Choose a reason for hiding this comment

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

Please

  • rename time_series to time_series_id
  • rename limit_row_count to limit
  • rename range_start to start
  • rename range_end to end
  • start and end should be flexible in terms of type - make a private function _parse_datetime(dt) that outputs the desired str format. Use pd.to_datetime and dt.isoformat() + 'Z'
  • use reasonable defaults for start = datetime.min and end = datetime.now()

Copy link
Member

@jsmariegaard jsmariegaard left a comment

Choose a reason for hiding this comment

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

Great work. A couple of comments though:

  • Tests are missing. I created a test file that you can populate with tests. Please add relevant tests
  • I guess you will delete the "if name == "main"" part when you're done?
  • See comments on get_data arguments

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