-
Notifications
You must be signed in to change notification settings - Fork 18
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
Keep geojson feature ids optional #595
Conversation
WalkthroughThe recent updates focus on improving the handling of the "id" field in GeoJSON features within the GeoJsonParser. By ensuring the presence and verifying the type of the "id" field as a string before any operations are performed, the changes significantly enhance the parser's ability to accurately process GeoJSON data, making the system more robust and reliable when dealing with diverse GeoJSON inputs. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -87,7 +87,7 @@ class GeoJsonParser { | |||
|
|||
const auto &properties = parseProperties(feature["properties"]); | |||
|
|||
if(feature["id"].is_string()) { | |||
if(feature.contains("id") && feature["id"].is_string()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to handle the "id" field checks for its existence and type correctly. However, consider extracting this logic into a separate method to improve readability and maintainability.
// Suggested refactor: Extract ID handling logic into a separate method.
std::string getFeatureId(const nlohmann::json &feature, UUIDGenerator &generator) {
if(feature.contains("id") && feature["id"].is_string()) {
return feature["id"].get<std::string>();
} else {
return std::to_string(generator.generateUUID());
}
}
// Usage within getGeoJson method:
geometry->featureContext = std::make_shared<FeatureContext>(geomType, properties, getFeatureId(feature, generator));
@@ -124,7 +124,7 @@ class GeoJsonParser { | |||
continue; | |||
} | |||
|
|||
if(!feature["id"].is_string()) { | |||
if(!feature.contains("id") || !feature["id"].is_string()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition to skip features without a string "id" field is correctly implemented according to the PR objectives. However, this approach might exclude valid GeoJSON features that do not have an "id" field, which could be against the intention of making "id" handling more flexible. Reconsider if skipping these features is the desired behavior.
Summary by CodeRabbit