Skip to content

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.

Sign up for GitHub

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

Closed
Shyam-123pandey wants to merge 3 commits into from
Closed

Remove comment-only and whitespace-only changes #11747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8bb52c
Add inline informational validation for regex-based fields
Shyam-123pandey Dec 31, 2025
48accd4
WIP: save crossing_ways fixes
Shyam-123pandey Jan 4, 2026
b39f94b
After removing commented code
Shyam-123pandey Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion css/30_highways.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
preset-icon-container/* highways */
/* preset-icon-container highways */

/* defaults */
.preset-icon .icon.tag-highway.other-line {
color: #fff;
fill: #777;
}

path.line.casing.tag-highway {
stroke: #444;
}

path.line.stroke.tag-highway {
stroke: #ccc;
}
Expand Down
66 changes: 58 additions & 8 deletions modules/ui/fields/textarea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,19 @@ import {
utilRebind
} from '../../util';
import { uiLengthIndicator } from '..';
import { svgIcon } from '../../svg/icon'; // ← ADDED


export function uiFieldTextarea(field, context) {
var dispatch = d3_dispatch('change');
var input = d3_select(null);
var wrap = d3_select(null); // ← ADDED
var _lengthIndicator = uiLengthIndicator(context.maxCharsForTagValue())
.silent(field.usage === 'changeset' && field.key === 'comment');
var _tags;


function textarea(selection) {
var wrap = selection.selectAll('.form-field-input-wrap')
wrap = selection.selectAll('.form-field-input-wrap')
.data([0]);

wrap = wrap.enter()
Expand All @@ -36,15 +37,24 @@ export function uiFieldTextarea(field, context) {
.attr('dir', 'auto')
.attr('id', field.domId)
.call(utilNoAuto)
.on('input', change(true))
.on('blur', change())
.on('change', change())
.on('input', function () {
change(true)();
updatePatternValidation(); // ← ADDED
})
.on('blur', function () {
change()();
updatePatternValidation(); // ← ADDED
})
.on('change', function () {
change()();
updatePatternValidation(); // ← ADDED
})
.merge(input);

wrap.call(_lengthIndicator);

function change(onInput) {
return function() {
return function () {

var val = utilGetSetValue(input);
if (!onInput) val = context.cleanTagValue(val);
Expand All @@ -58,9 +68,47 @@ export function uiFieldTextarea(field, context) {
};
}
}

Copy link

Copilot AI Jan 7, 2026

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.

Copilot uses AI. Check for mistakes.
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 {
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
} 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 */

Copilot uses AI. Check for mistakes.
return;
}

const info = wrap.selectAll('.form-field-pattern-info')
.data(isInvalid ? [0] : []);

const enter = info.enter()
Comment on lines +75 to +93
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
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()

Copilot uses AI. Check for mistakes.
.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();
}
Comment on lines +72 to +107
Copy link

Copilot AI Jan 7, 2026

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:

  1. Should be explicitly documented in the PR description
  2. Requires test coverage
  3. 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.

Copilot uses AI. Check for mistakes.
// ============================
Copy link

Copilot AI Jan 7, 2026

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").

Suggested change
// ============================
// Public API methods

Copilot uses AI. Check for mistakes.


textarea.tags = function(tags) {
textarea.tags = function (tags) {
_tags = tags;

var isMixed = Array.isArray(tags[field.key]);
Expand All @@ -73,10 +121,12 @@ export function uiFieldTextarea(field, context) {
if (!isMixed) {
_lengthIndicator.update(tags[field.key]);
}

updatePatternValidation();
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
updatePatternValidation();
updatePatternValidation();

Copilot uses AI. Check for mistakes.
};


textarea.focus = function() {
textarea.focus = function () {
input.node().focus();
};

Expand Down
45 changes: 28 additions & 17 deletions modules/validations/crossing_ways.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ export function validationCrossingWays(context) {
return fix;
}

function makeChangeLayerFix(higherOrLower) {
function makeChangeLayerFix(higherOrLower) {
return new validationIssueFix({
icon: 'iD-icon-' + (higherOrLower === 'higher' ? 'up' : 'down'),
title: t.append('issues.fix.tag_this_as_' + higherOrLower + '.title'),
Expand All @@ -807,29 +807,39 @@ export function validationCrossingWays(context) {
if (selectedIDs.length !== 1) return;

var selectedID = selectedIDs[0];
if (!this.issue.entityIds.some(function(entityId) {
return entityId === selectedID;
})) return;
if (!this.issue.entityIds.includes(selectedID)) return;

var entity = context.hasEntity(selectedID);
if (!entity) return;

var tags = Object.assign({}, entity.tags); // shallow copy
var layer = tags.layer && Number(tags.layer);
if (layer && !isNaN(layer)) {
if (higherOrLower === 'higher') {
layer += 1;
} else {
layer -= 1;
}
// Clone tags (never mutate original)
var tags = Object.assign({}, entity.tags);

// Determine feature type
var featureType = getFeatureType(entity, context.graph());

// Compute new layer value
var layer = Number(tags.layer);
if (!isNaN(layer)) {
layer += (higherOrLower === 'higher' ? 1 : -1);
Comment on lines +822 to +824
Copy link

Copilot AI Jan 7, 2026

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
} else {
if (higherOrLower === 'higher') {
layer = 1;
} else {
layer = -1;
}
layer = (higherOrLower === 'higher' ? 1 : -1);
}
tags.layer = layer.toString();

// 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;
}
Comment on lines +830 to +841
Copy link

Copilot AI Jan 7, 2026

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:

  1. Explicitly documented in the PR description
  2. Covered by tests
  3. Reviewed as a feature addition, not as a refactoring

Consider splitting this into a separate PR with appropriate tests and documentation.

Copilot uses AI. Check for mistakes.
Comment on lines +830 to +841
Copy link

Copilot AI Jan 7, 2026

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:

  1. Bridge tags are correctly added when moving to higher layers
  2. Tunnel tags are correctly added when moving to lower layers
  3. Tags are correctly removed for unsupported feature types
  4. Bridge and tunnel tags are mutually exclusive

Copilot uses AI. Check for mistakes.

context.perform(
actionChangeTags(entity.id, tags),
t('operations.change_tags.annotation')
Expand All @@ -838,6 +848,7 @@ export function validationCrossingWays(context) {
});
}


validation.type = type;

return validation;
Expand Down