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

Updated Frequency Module #432

Merged
merged 13 commits into from
Mar 23, 2020
Merged

Updated Frequency Module #432

merged 13 commits into from
Mar 23, 2020

Conversation

rgao
Copy link
Contributor

@rgao rgao commented Mar 16, 2020

Updated frequency module to return data in desired format

  • returns array of start dates of each bin
  • returns the corresponding number of requests for each request type under each bin

Known issues:

  • bin start dates are displaying backwards, starting from most recent bucket
  • inefficiency with using data service & accounting for timeframe input

Copy link
Member

@sellnat77 sellnat77 left a comment

Choose a reason for hiding this comment

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

Looks like there was a bad merge
Might want to reset files that you didn't intend on changing

@rgao
Copy link
Contributor Author

rgao commented Mar 17, 2020

Yea, I fixed the bad merge issues in app.py. Is the Terraform failed check something I can work on?

Also, something in my code broke regarding data types despite that I didn't change anything in the module; the json response is not going through... gotta fix that

EDIT: fixed the merge issues and json response issue, pushing again
EDIT2: fixed linting errors

@sellnat77 : I think it should be OK to merge now

@sellnat77
Copy link
Member

I'm guessing the terrain issues are failing because you're using a forked repo, your repo doesn't have the required credentials to perform the terrain plan

} for request in requestTypes]
}]

# Following is deprecated, saving for reference
Copy link
Member

Choose a reason for hiding this comment

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

We can remove all this commented stuff
If we need to reference it we can go through git-history. Otherwise this will make the code messy

@@ -0,0 +1,68 @@
from configparser import ConfigParser
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this file could be defunkt, but we can keep it for now. Lets drop in a comment with the link to the GH issue regarding the precaching

I have a feeling this will become necessary when we start hitting massive scale

@compress.compress()
async def requestfrequency(request):
freq_worker = frequency(app.config['Settings'])
freq_worker = FrequencyService(app.config['Settings'])
Copy link
Member

Choose a reason for hiding this comment

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

Right now this is using hardcoded values, let throw in the logic to pull parameters from the request object

@sellnat77 sellnat77 self-requested a review March 23, 2020 05:38
Copy link
Member

@sellnat77 sellnat77 left a comment

Choose a reason for hiding this comment

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

We can merge this to unblock others, buts lets create some tech debt tickets to make sure we clean things up later on

@sellnat77 sellnat77 merged commit c88655d into hackforla:dev Mar 23, 2020
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.

2 participants