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

#3292 ClusterSize & #3293 Neighborhood Name #3323

Merged

Conversation

kevinjxc
Copy link
Collaborator

Resolves #3292, #3293

Add clusterSize property, streamline region variables to neighborhood variables.

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 @1kechen! Just a few requests in addition to the comments:

  • Looks like you made the changes for the JSON and CSV APIs, but not for the Shapefile APIs. You should make the changes there too! It's probably easiest to test the shapefiles by looking for some website that will visualize small shapefiles for you :)
  • Are you creating the PR through the Github website or on the command line? If you're not already, you should do it through the website and use the provided template!
  • Next time I would suggest splitting this into two separate PRs so that we can merge one of them even if there are problems with the other, but this is fine for now!

app/controllers/ProjectSidewalkAPIController.scala Outdated Show resolved Hide resolved
app/controllers/RegionController.scala Outdated Show resolved Hide resolved
app/models/attribute/GlobalAttributeTable.scala Outdated Show resolved Hide resolved
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.

Added one comment in the code. Also looks like you forgot to change from region to neighborhood in the shapefiles!

app/controllers/ProjectSidewalkAPIController.scala Outdated Show resolved Hide resolved
@kevinjxc kevinjxc force-pushed the 3292-3293-neighborhood-name-and-cluster-size-properties branch from 0d23a8a to 7213090 Compare August 18, 2023 23:13
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.

@1kechen Did you check that the shapefiles worked? The max number of characters for a field in a shapefile is 10 characters. Your field names "clusterSize" and "neighborhoodID" had 11 and 14 characters respectively, which corrupted the actual data and turned it to null. I've renamed them to "clusterSze" and "nghborhdId" and they work now.

Please make sure to test all of your code!

@misaugstad misaugstad merged commit e0c62e2 into develop Sep 11, 2023
@misaugstad misaugstad deleted the 3292-3293-neighborhood-name-and-cluster-size-properties branch September 11, 2023 19:43
@misaugstad misaugstad mentioned this pull request Sep 29, 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.

Add clusterSize property to API
2 participants