Skip to content

Added esupgrade pre-commit hook #2421

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

Open
codingjoe wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Added esupgrade pre-commit hook #2421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ceaa05
Add esupgrade pre-commit hook
codingjoe Jan 18, 2026
26dd61a
Apply esupgrade changes (baseline widly-available)
codingjoe Jan 18, 2026
3db9193
Apply co-pilot review suggestions
codingjoe Dec 30, 2025
416f0d2
Unloop cookie method
codingjoe Dec 30, 2025
684f527
Use console log levels (baseline: widely-available)
codingjoe Dec 30, 2025
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
6 changes: 6 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ repos:
- id: django-upgrade
args: [--target-version, "6.0"]

- repo: https://github.com/codingjoe/esupgrade
Copy link
Member

@adamzap adamzap Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No offense, @codingjoe, but I'm not sure if a project like Django's website should be relying on a project with seven GitHub stars. During review I assumed that esupgrade was a well-known code formatting tool. I'm curious to hear what other members of the working group think though.

Copy link
Member

@ulgens ulgens Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully support the goal and the intention of this PR, but I agree that the tool not being widely-accepted yet is a bit concerning. I don't have a strong opinion, but I'd recommend selecting something more established (if there is any) or waiting a bit to see how esupgrade progresses.

Copy link
Contributor Author

@codingjoe codingjoe Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None taken; this was inspired by a conversation on Mastodon and meant as a testing ground to harden esupgrade.

I can understand hesitance to introduce this tool to the dev toolchain, and I don't mind either way.

However, I reviewed all automatic changes and, of course, my own in the latter commits.
So if those changes find appreciation, I am happy to make this a one-time thing.

Copy link
Member

@ulgens ulgens Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the tool's maturity, +1 for merging useful changes from the PR.

rev: 2025.14.3
hooks:
- id: esupgrade
exclude: '(^djangoproject\/static\/js\/lib\/.*$)'

- repo: https://github.com/psf/black-pre-commit-mirror
rev: 25.12.0
hooks:
Expand Down
28 changes: 14 additions & 14 deletions djangoproject/static/js/admin_blog_imageupload.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
document.addEventListener('DOMContentLoaded', function () {
document
.querySelectorAll('button[data-clipboard-content]')
.forEach(function (el) {
el.addEventListener('click', function (e) {
// Copy to clipboard and flash the button for a second
navigator.clipboard.writeText(el.dataset.clipboardContent);
el.style.backgroundColor = 'rebeccapurple';
el.style.color = 'white';
window.setTimeout(function () {
el.style.backgroundColor = '';
el.style.color = '';
}, 1000);
});
document.addEventListener('DOMContentLoaded', () => {
for (const el of document.querySelectorAll(
'button[data-clipboard-content]',
)) {
el.addEventListener('click', (e) => {
// Copy to clipboard and flash the button for a second
navigator.clipboard.writeText(el.dataset.clipboardContent);
el.style.backgroundColor = 'rebeccapurple';
el.style.color = 'white';
globalThis.setTimeout(() => {
el.style.backgroundColor = '';
el.style.color = '';
}, 1000);
});
}
});
43 changes: 19 additions & 24 deletions djangoproject/static/js/dashboard/detail.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
$(function () {
var element = $('#graph');
var url = element.data('path') + element.data('metric') + '.json?days=365';
var hover = {
show: function (x, y, message) {
$(() => {
const element = $('#graph');
const url = `${element.data('path') + element.data('metric')}.json?days=365`;
Copy link
Member

@adamzap adamzap Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be nicer and more consistent as:

`${element.data('path')}${element.data('metric')}.json?days=365`

Copy link
Contributor Author

@codingjoe codingjoe Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this transformation isn't automated by esupgrade, since we don't know that element.data doesn't return an integer (where addition would work differently).

const hover = {
show: (x, y, message) => {
$('<div id="hover">')
.html(message)
.css({ top: y, left: x })
.appendTo('body')
.show();
},
hide: function () {
hide: () => {
$('#hover').remove();
},
};

$.getJSON(url, function (response) {
for (var i = 0; i < response.data.length; i++) {
$.getJSON(url, (response) => {
for (let i = 0; i < response.data.length; i++) {
response.data[i][0] = response.data[i][0] * 1000;
}
var options = {
const options = {
xaxis: {
mode: 'time',
tickColor: 'rgba(0,0,0,0)',
Expand All @@ -41,27 +41,22 @@ $(function () {
align: 'center',
};
}
var plot = $.plot(element, [response.data], options);
$.plot(element, [response.data], options);

var format_message = function (timestamp, measurement) {
var unit = measurement == 1 ? response.unit : response.unit_plural;
return (
formatTimestamp(timestamp, response.period) +
'<br>' +
measurement +
' ' +
unit
);
};
function format_message(timestamp, measurement) {
const unit = measurement == 1 ? response.unit : response.unit_plural;
Copy link
Member

@adamzap adamzap Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be ===? Maybe that's out of scope here...

Copy link
Contributor Author

@codingjoe codingjoe Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are numerous places with incorrect equality checks. This is a case for ESLint.

return `${formatTimestamp(timestamp, response.period)}<br>${measurement} ${unit}`;
Copy link
Member

@adamzap adamzap Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to enforce a maximum line length? These files make me think it was 80 previously.

Copy link
Contributor Author

@codingjoe codingjoe Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm… yes, more recent versions of esupgrade keep multiline concatenation.
I think this should probably be set up in a linter/formatter (which esupgrade is not).

}

var previousPoint = null;
element.bind('plothover', function (event, pos, item) {
let previousPoint = null;
element.bind('plothover', (event, pos, item) => {
if (item) {
if (previousPoint != item.dataIndex) {
previousPoint = item.dataIndex;
hover.hide();
var x, y;
var message = format_message.apply(null, item.datapoint);
let x;
let y;
const message = format_message.apply(null, item.datapoint);
if (response.period == 'instant') {
x = item.pageX + 10;
y = item.pageY + 10;
Expand Down
20 changes: 10 additions & 10 deletions djangoproject/static/js/dashboard/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
$(function () {
$('.metric .sparkline').each(function (index, elem) {
var element = $(elem);
var valueElement = element.parent().find('.value a');
var timestampElement = element.parent().find('.timestamp');
var originalValue = valueElement.html();
var green = '#93D7B7';
$(() => {
$('.metric .sparkline').each((index, elem) => {
const element = $(elem);
const valueElement = element.parent().find('.value a');
const timestampElement = element.parent().find('.timestamp');
const originalValue = valueElement.html();
const green = '#93D7B7';

var url = element.data('path') + element.data('metric') + '.json';
$.getJSON(url, function (response) {
const url = `${element.data('path') + element.data('metric')}.json`;
Copy link
Member

@adamzap adamzap Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as above.

$.getJSON(url, (response) => {
response.data = convertSecondsToMilliseconds(response.data);
$.plot(element, [response.data], {
xaxis: { show: false, mode: 'time' },
Expand All @@ -26,7 +26,7 @@ $(function () {
},
});

element.bind('plothover', function (event, pos, item) {
element.bind('plothover', (event, pos, item) => {
if (item) {
valueElement.html(item.datapoint[1]);
timestampElement.html(
Expand Down
14 changes: 5 additions & 9 deletions djangoproject/static/js/dashboard/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
function formatTimestamp(timestamp, period) {
var d = new Date(timestamp);
const d = new Date(timestamp);
if (period == 'instant') {
return $.plot.formatDate(d, '%b %d, %h:%M %p');
} else if (period == 'daily') {
Expand All @@ -8,18 +8,14 @@ function formatTimestamp(timestamp, period) {
// A bit more complicated than the above: the timestamp is in the
// middle of the week, so we have to bracket the date. This is
// something of a fudge here, but it works well enough.
var start = new Date(d.getTime() - 3 * 24 * 60 * 60 * 1000);
var end = new Date(d.getTime() + 3 * 24 * 60 * 60 * 1000);
return (
$.plot.formatDate(start, '%b %d') +
' - ' +
$.plot.formatDate(end, '%b %d')
);
const start = new Date(d.getTime() - 3 * 24 * 60 * 60 * 1000);
const end = new Date(d.getTime() + 3 * 24 * 60 * 60 * 1000);
return `${$.plot.formatDate(start, '%b %d')} - ${$.plot.formatDate(end, '%b %d')}`;
}
}

function convertSecondsToMilliseconds(data) {
for (var i = 0; i < data.length; i++) {
for (let i = 0; i < data.length; i++) {
data[i][0] = data[i][0] * 1000;
}
return data;
Expand Down
60 changes: 30 additions & 30 deletions djangoproject/static/js/djangoproject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1,45 @@
// Toggle persistent display of documentation version and language options
document.querySelectorAll('.doc-switcher li.current').forEach(function (el) {
for (const el of document.querySelectorAll('.doc-switcher li.current')) {
Copy link
Member

@adamzap adamzap Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a for loop better here for some reason, or are we just conforming to a certain style?

Copy link
Contributor Author

@codingjoe codingjoe Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously mentioned. There is nothing wrong with it. However, the new features of ECMAScript have a history of why their proposal was drafted and included in an ES release.

In other words, yes, there are opinions baked into the esupgrade, including my own. But since I come from a Python background, I'd presume some might be welcome here.

el.addEventListener('click', function () {
this.parentElement.classList.toggle('open');
});
});
}

// Propagate the current fragment identifier when switching docs versions
document.querySelectorAll('#doc-versions a').forEach(function (el) {
for (const el of document.querySelectorAll('#doc-versions a')) {
el.addEventListener('click', function () {
this.href = this.href.split('#')[0] + window.location.hash;
this.href = this.href.split('#')[0] + globalThis.location.hash;
});
});
}

// Fade out and remove message elements when close icon is clicked
document.querySelectorAll('.messages li .close').forEach(function (el) {
for (const el of document.querySelectorAll('.messages li .close')) {
el.addEventListener('click', function () {
this.parentElement.addEventListener('transitionend', function () {
this.style.display = 'none';
});

this.parentElement.classList.add('fade-out');
});
});
}

// Check all console tab inputs of the same type when one's label is clicked
document.querySelectorAll('.console-block label').forEach(function (el) {
el.addEventListener('click', function (e) {
for (const el of document.querySelectorAll('.console-block label')) {
el.addEventListener('click', (e) => {
const input_id = e.currentTarget.getAttribute('for');
const selector = input_id.endsWith('unix') ? '.c-tab-unix' : '.c-tab-win';

document.querySelectorAll(selector).forEach(function (input_el) {
for (const input_el of document.querySelectorAll(selector)) {
input_el.checked = true;
});
}
});
});
}

// Add animation class to feature icons when they are fully visible
(function () {
(() => {
const observer = new IntersectionObserver(
function (entries) {
entries.forEach(function (entry) {
(entries) => {
entries.forEach((entry) => {
if (!entry.isIntersecting) {
return;
}
Expand All @@ -52,9 +52,9 @@ document.querySelectorAll('.console-block label').forEach(function (el) {
{ threshold: 1.0 },
);

document.querySelectorAll('.list-features i').forEach(function (el) {
for (const el of document.querySelectorAll('.list-features i')) {
observer.observe(el);
});
}
})();

// Toggle mobile menu on button click
Expand All @@ -66,7 +66,7 @@ document.querySelector('.menu-button').addEventListener('click', function () {
});

// Update search input placeholder text based on the user's operating system
(function () {
(() => {
const el = document.getElementById('id_q');

if (!el) {
Expand All @@ -81,7 +81,7 @@ document.querySelector('.menu-button').addEventListener('click', function () {
})();

// Focus, select, and scroll to search input when key combination is pressed
window.addEventListener('keydown', function (e) {
globalThis.addEventListener('keydown', (e) => {
const is_ctrl_k = (e.metaKey || e.ctrlKey) && e.key === 'k';

if (!(is_ctrl_k || e.key === '/')) {
Expand All @@ -99,11 +99,11 @@ window.addEventListener('keydown', function (e) {
el.select();
el.focus();

window.scrollTo({ top: 0, left: 0, behavior: 'smooth' });
globalThis.scrollTo({ top: 0, left: 0, behavior: 'smooth' });
});

// Add copy buttons to code snippets
(function () {
(() => {
const button_el = document.createElement('span');

button_el.classList.add('btn-clipboard');
Expand All @@ -112,13 +112,13 @@ window.addEventListener('keydown', function (e) {

const selector = '.snippet-filename, .code-block-caption';

document.querySelectorAll(selector).forEach(function (el) {
for (const el of document.querySelectorAll(selector)) {
el.insertBefore(button_el.cloneNode(true), null);
});
}
})();

// Attach copy functionality to dynamically-created buttons
document.querySelectorAll('.btn-clipboard').forEach(function (el) {
for (const el of document.querySelectorAll('.btn-clipboard')) {
el.addEventListener('click', function () {
const success_el = document.createElement('span');

Expand All @@ -133,15 +133,15 @@ document.querySelectorAll('.btn-clipboard').forEach(function (el) {
function on_success(el) {
success_el.innerText = 'Copied!';

setTimeout(function () {
setTimeout(() => {
success_el.classList.add('fade-out');
}, 1000);
}

function on_error(el) {
success_el.innerText = 'Could not copy!';

setTimeout(function () {
setTimeout(() => {
success_el.classList.add('fade-out');
}, 5000);
}
Expand All @@ -150,10 +150,10 @@ document.querySelectorAll('.btn-clipboard').forEach(function (el) {

navigator.clipboard.writeText(text).then(on_success, on_error);
});
});
}

// Update donate button text on fundraising page based on interval selection
(function () {
(() => {
const el = document.querySelector('#donate #id_interval');

if (!el) {
Expand All @@ -168,7 +168,7 @@ document.querySelectorAll('.btn-clipboard').forEach(function (el) {
})();

// Manage custom donation amount input on fundraising page
(function () {
(() => {
const el = document.querySelector('#donate #id_amount');

if (!el) {
Expand Down Expand Up @@ -198,7 +198,7 @@ document.querySelectorAll('.btn-clipboard').forEach(function (el) {
})();

// Manage amount and membership level fields on corporate membership page
(function () {
(() => {
const form_el = document.querySelector('.corporate-membership-join-form');

if (!form_el) {
Expand Down
Loading