-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Remove comment-only and whitespace-only changes #11747
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
Remove comment-only and whitespace-only changes #11747
Conversation
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.
Pull request overview
This PR claims to remove comment-only and whitespace-only changes, but actually introduces significant new functionality alongside formatting fixes. The main additions include pattern validation for textarea fields and automatic bridge/tunnel tagging when changing feature layers.
Key changes:
- Added pattern validation functionality to textarea fields with visual feedback
- Implemented automatic bridge/tunnel tagging when adjusting layer values in crossing ways validation
- Fixed indentation and comment formatting issues
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| modules/validations/crossing_ways.js | Fixed indentation, modernized array method, refactored layer logic, and added new bridge/tunnel tagging behavior based on layer changes |
| modules/ui/fields/textarea.js | Added pattern validation function with visual feedback, updated event handlers to support validation, and added new imports |
| css/30_highways.css | Fixed malformed comment syntax and added whitespace for readability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Apply bridge/tunnel ONLY to allowed linear features | ||
| if (featureType && allowsBridge(featureType) && higherOrLower === 'higher') { | ||
| tags.bridge = 'yes'; | ||
| delete tags.tunnel; | ||
| } else if (featureType && allowsTunnel(featureType) && higherOrLower === 'lower') { | ||
| tags.tunnel = 'yes'; | ||
| delete tags.bridge; | ||
| } else { | ||
| // Buildings or unsupported features | ||
| delete tags.bridge; | ||
| delete tags.tunnel; | ||
| } |
Copilot
AI
Jan 7, 2026
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 PR description states "No functional or behavioral changes are intended", but this change introduces significant new functionality. The code now automatically adds or removes bridge/tunnel tags based on the layer direction and feature type, which is a behavioral change that should be:
- Explicitly documented in the PR description
- Covered by tests
- Reviewed as a feature addition, not as a refactoring
Consider splitting this into a separate PR with appropriate tests and documentation.
| try { | ||
| const regex = new RegExp(field.pattern); | ||
| isInvalid = !regex.test(value); | ||
| } catch { |
Copilot
AI
Jan 7, 2026
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 catch block silently ignores all errors without any logging or handling. If the regex construction fails due to an invalid pattern, this should at least be logged for debugging purposes. Consider logging the error or adding a comment explaining why silent failure is acceptable here.
| } catch { | |
| } catch (err) { | |
| // Log invalid regex patterns for debugging while preserving existing behavior | |
| /* eslint-disable no-console */ | |
| console.error('uiFieldTextarea: invalid regex pattern for field.pattern', field.pattern, err); | |
| /* eslint-enable no-console */ |
| // Apply bridge/tunnel ONLY to allowed linear features | ||
| if (featureType && allowsBridge(featureType) && higherOrLower === 'higher') { | ||
| tags.bridge = 'yes'; | ||
| delete tags.tunnel; | ||
| } else if (featureType && allowsTunnel(featureType) && higherOrLower === 'lower') { | ||
| tags.tunnel = 'yes'; | ||
| delete tags.bridge; | ||
| } else { | ||
| // Buildings or unsupported features | ||
| delete tags.bridge; | ||
| delete tags.tunnel; | ||
| } |
Copilot
AI
Jan 7, 2026
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 new logic that automatically adds bridge='yes' or tunnel='yes' tags when changing layers lacks test coverage. Given that test/spec/validations/crossing_ways.js exists and tests layer-related behavior, this new functionality should be tested to ensure:
- Bridge tags are correctly added when moving to higher layers
- Tunnel tags are correctly added when moving to lower layers
- Tags are correctly removed for unsupported feature types
- Bridge and tunnel tags are mutually exclusive
| _lengthIndicator.update(tags[field.key]); | ||
| } | ||
|
|
||
| updatePatternValidation(); |
Copilot
AI
Jan 7, 2026
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.
This line contains trailing whitespace. Remove the trailing spaces for code cleanliness.
| updatePatternValidation(); | |
| updatePatternValidation(); |
| const value = utilGetSetValue(input).trim(); | ||
|
|
||
| if (!value) { | ||
| wrap.selectAll('.form-field-pattern-info').remove(); | ||
| return; | ||
| } | ||
|
|
||
| let isInvalid = false; | ||
| try { | ||
| const regex = new RegExp(field.pattern); | ||
| isInvalid = !regex.test(value); | ||
| } catch { | ||
| return; | ||
| } | ||
|
|
||
| const info = wrap.selectAll('.form-field-pattern-info') | ||
| .data(isInvalid ? [0] : []); | ||
|
|
||
| const enter = info.enter() |
Copilot
AI
Jan 7, 2026
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 new function uses modern JavaScript variable declarations (const/let) while the rest of the file consistently uses var. For consistency with the existing codebase style, consider using var instead of const/let throughout this function, or update the entire file to use modern declarations in a separate refactoring PR.
| const value = utilGetSetValue(input).trim(); | |
| if (!value) { | |
| wrap.selectAll('.form-field-pattern-info').remove(); | |
| return; | |
| } | |
| let isInvalid = false; | |
| try { | |
| const regex = new RegExp(field.pattern); | |
| isInvalid = !regex.test(value); | |
| } catch { | |
| return; | |
| } | |
| const info = wrap.selectAll('.form-field-pattern-info') | |
| .data(isInvalid ? [0] : []); | |
| const enter = info.enter() | |
| var value = utilGetSetValue(input).trim(); | |
| if (!value) { | |
| wrap.selectAll('.form-field-pattern-info').remove(); | |
| return; | |
| } | |
| var isInvalid = false; | |
| try { | |
| var regex = new RegExp(field.pattern); | |
| isInvalid = !regex.test(value); | |
| } catch { | |
| return; | |
| } | |
| var info = wrap.selectAll('.form-field-pattern-info') | |
| .data(isInvalid ? [0] : []); | |
| var enter = info.enter() |
| var layer = Number(tags.layer); | ||
| if (!isNaN(layer)) { | ||
| layer += (higherOrLower === 'higher' ? 1 : -1); |
Copilot
AI
Jan 7, 2026
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 code now uses Number(tags.layer) without checking if tags.layer exists first. If tags.layer is undefined, Number(undefined) returns NaN, which is handled by the isNaN check. However, this is less explicit than the original code which checked tags.layer && before calling Number. Consider adding a comment explaining this intentional change, or revert to the more explicit pattern for clarity.
| var layer = Number(tags.layer); | |
| if (!isNaN(layer)) { | |
| layer += (higherOrLower === 'higher' ? 1 : -1); | |
| var rawLayer = tags.layer; | |
| var layer; | |
| if (rawLayer && !isNaN(Number(rawLayer))) { | |
| layer = Number(rawLayer) + (higherOrLower === 'higher' ? 1 : -1); |
| function updatePatternValidation() { | ||
| if (!field.pattern || !wrap || wrap.empty()) return; | ||
|
|
||
| const value = utilGetSetValue(input).trim(); | ||
|
|
||
| if (!value) { | ||
| wrap.selectAll('.form-field-pattern-info').remove(); | ||
| return; | ||
| } | ||
|
|
||
| let isInvalid = false; | ||
| try { | ||
| const regex = new RegExp(field.pattern); | ||
| isInvalid = !regex.test(value); | ||
| } catch { | ||
| return; | ||
| } | ||
|
|
||
| const info = wrap.selectAll('.form-field-pattern-info') | ||
| .data(isInvalid ? [0] : []); | ||
|
|
||
| const enter = info.enter() | ||
| .append('div') | ||
| .attr('class', 'form-field-pattern-info'); | ||
|
|
||
| enter | ||
| .append('span') | ||
| .call(svgIcon('#iD-icon-info')); | ||
|
|
||
| enter | ||
| .append('span') | ||
| .attr('class', 'form-field-pattern-info-text') | ||
| .text(t('inspector.invalid_format')); | ||
|
|
||
| info.exit().remove(); | ||
| } |
Copilot
AI
Jan 7, 2026
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 PR description states "No functional or behavioral changes are intended", but this change introduces new pattern validation functionality for textarea fields. This is a significant feature addition that:
- Should be explicitly documented in the PR description
- Requires test coverage
- Should be reviewed as a feature addition, not as a refactoring
Consider splitting this into a separate PR with appropriate tests and documentation explaining when and how pattern validation works.
|
|
||
| info.exit().remove(); | ||
| } | ||
| // ============================ |
Copilot
AI
Jan 7, 2026
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.
This comment line appears to be a separator or section marker but doesn't provide meaningful information. Consider removing it or replacing it with a more descriptive comment explaining what follows (e.g., "// Public API methods").
| // ============================ | |
| // Public API methods |
| }; | ||
| } | ||
| } | ||
|
|
Copilot
AI
Jan 7, 2026
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.
This line contains trailing whitespace. Remove the trailing spaces for code cleanliness.
This commit removes comment-only and whitespace-only changes that were
introduced during drafting.
This is improved version of things, like previously i only changed in textarea.js but for more improvement some changes in validation also.
No functional or behavioral changes are intended.
#11736