feat: add location data with schema validation#15
Conversation
Add functionality to fetch and validate location data with a consistent schema. ## Changes - Add getLocations() function with schema validation - Validate required fields (id, name) - Normalize optional fields with null defaults - Support custom fields in location objects - Add comprehensive test suite - Update README with schema documentation ## Features - Fetch locations.json with consistent schema - Validate each location against schema - Return both valid locations and validation errors - Support custom file names and branches - Preserve custom fields while normalizing core fields - what3words, coordinates, accessibility support 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds location management functionality to the gitevents-fetch library, enabling users to fetch and validate venue/location data from GitHub repositories. The implementation introduces a new getLocations API function with schema validation and comprehensive test coverage.
Key Changes:
- Added
getLocationsfunction with location schema validation (id, name, coordinates, etc.) - Comprehensive test suite covering parameter validation, data normalization, custom fields, and error handling
- Updated package version from 0.0.3-dev to 0.0.4
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/locations.js | Implements location fetching and validation logic with schema normalization |
| src/index.js | Exports the new getLocations function as part of the public API |
| test/locations.test.js | Comprehensive test suite covering all location functionality scenarios |
| package.json | Version bump to 0.0.4 |
| package-lock.json | Lock file updated for version 0.0.4 |
| README.md | Documentation for the new getLocations API including schema details and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,131 @@ | |||
| import { getFile } from './files.js' | |||
There was a problem hiding this comment.
The imported module './files.js' does not exist in the codebase. This will cause a runtime error when the getLocations function is called. Either create the missing files.js module or replace this with the appropriate file-fetching implementation using the graphql client directly.
| if (location.coordinates) { | ||
| if (typeof location.coordinates !== 'object') { | ||
| errors.push('Location coordinates must be an object') | ||
| } else { | ||
| if (typeof location.coordinates.lat !== 'number') { | ||
| errors.push('Location coordinates.lat must be a number') | ||
| } | ||
| if (typeof location.coordinates.lng !== 'number') { | ||
| errors.push('Location coordinates.lng must be a number') | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation logic doesn't check if both lat and lng exist when coordinates is provided. If coordinates is an object but missing lat or lng properties, the validation will pass (no error added) but lines 86-87 will attempt to destructure undefined values, resulting in a location object with { lat: undefined, lng: undefined } instead of null.
test/locations.test.js
Outdated
| import assert from 'node:assert' | ||
| import { getLocations } from '../src/locations.js' | ||
|
|
||
| // Mock getFile function |
There was a problem hiding this comment.
The comment states 'Mock getFile function' but the code that follows actually mocks the graphql function, not getFile. Update comment to 'Mock graphql function for file fetching' to accurately describe what createMockGraphql does.
| // Mock getFile function | |
| // Mock graphql function for file fetching |
- Add explicit checks for lat/lng existence in coordinates object - Update test comment to accurately describe mocking graphql function - Add test case for missing lat or lng in coordinates Addresses Copilot review comments in PR #15
Review UpdatesAddressed Copilot review comments: ✅ Fixed coordinates validation - Added explicit checks for missing ✅ Fixed test comment - Updated comment to accurately describe mocking graphql function instead of getFile ✅ Added test case - Added comprehensive test for missing lat/lng in coordinates Dependency Note
The Suggested merge order:
Alternatively, all file-fetching related PRs (#5, #12) can be reviewed together as they form the foundation for this location data feature. |
Resolved conflicts between location feature and new main features: - Combined getLocations, getUser, and getFile imports in src/index.js - Merged API documentation for all three functions in README.md - Added parameter validation to getLocations for consistency - All 59 tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| function validateParams(params) { | ||
| const missing = [] | ||
| for (const [key, value] of Object.entries(params)) { | ||
| if (!value) missing.push(key) | ||
| } | ||
| if (missing.length > 0) { | ||
| throw new Error(`Missing required parameters: ${missing.join(', ')}`) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicated code: The validateParams function is duplicated from src/lib/validateParams.js. Import and use the existing utility function instead of redefining it.
| function validateParams(params) { | |
| const missing = [] | |
| for (const [key, value] of Object.entries(params)) { | |
| if (!value) missing.push(key) | |
| } | |
| if (missing.length > 0) { | |
| throw new Error(`Missing required parameters: ${missing.join(', ')}`) | |
| } | |
| } | |
| import { validateParams } from './lib/validateParams.js' |
| url: location.url || null, | ||
| what3words: location.what3words || null, | ||
| description: location.description || null, | ||
| capacity: location.capacity || null, |
There was a problem hiding this comment.
Incorrect falsy value handling: Using || will convert capacity: 0 to null. Use nullish coalescing (??) instead to preserve zero values.
| capacity: location.capacity || null, | |
| capacity: location.capacity ?? null, |
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Adds location data fetching with schema validation for consistent
venue/location data structure.
Closes #9
Changes
API Usage
Location Schema
Required fields:
id(string) - Unique identifiername(string) - Location nameOptional fields:
address(string) - Physical addresscoordinates(object) - { lat: number, lng: number }url(string) - Location websitewhat3words(string) - what3words addressdescription(string) - Descriptioncapacity(number) - Venue capacityaccessibility(string) - Accessibility infoFeatures
Test Plan
🤖 Generated with Claude Code