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

Added launch date to overallStats API call #3396

Merged
merged 4 commits into from
Oct 23, 2023
Merged

Conversation

davphan
Copy link
Collaborator

@davphan davphan commented Oct 2, 2023

Resolves #3375

Launch date of a city is currently not available. To show this information, the date of the first label in each city was found and added to the city params config file. The info is then drawn from the config file to the API call.

Before/After screenshots (if applicable)

Before:
No info displayed for city launch date.

After:
CSV format:
image

JSON format:
image

Testing instructions
  1. Use the /v2/overallStats and /v2/overallStats?filetype=csv API calls.
  2. Check that the launch_date/Launch Date fields are displayed correctly.
Things to check before submitting the PR
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Thanks, just a quick restructuring thing to do!

app/controllers/ProjectSidewalkAPIController.scala Outdated Show resolved Hide resolved
conf/cityparams.conf Show resolved Hide resolved
@davphan
Copy link
Collaborator Author

davphan commented Oct 13, 2023

@misaugstad Ready for another review!

Copy link
Member

@misaugstad misaugstad 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, thanks @davphan!

I did make two minor changes that you should look out for in the future.

  1. There was an import statement left over from before the recent refactor. You can make sure stuff like that doesn't happen by looking over the "files changed" tab of your PR after you finish making your edits. It's a good idea to read through all those changes to make sure there's no extra fluff that you left in!
  2. I added type annotations to a couple variables. This is strongly encouraged in Scala. I'd generally like for you to use them almost everywhere. One exception is for Slick query objects; those can have really long, uninformative data types. The other exception is for edge cases where it would significantly decrease the readability of the code, while the data type is obvious from context. What I changed were the two Play.configuration.getString lines to make them look like val cityId: String = ..., etc.

@misaugstad misaugstad merged commit 0987a50 into develop Oct 23, 2023
@misaugstad misaugstad deleted the 3375-launch-date-API branch October 23, 2023 19:21
@misaugstad misaugstad mentioned this pull request Nov 1, 2023
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.

Launch date of each city server in API?
2 participants