Skip to content

Fixes #39383 - add no-magic-numbers js lint rule#11008

Merged
stejskalleos merged 1 commit into
theforeman:developfrom
MariaAga:lint-magic-numbers
Jul 13, 2026
Merged

Fixes #39383 - add no-magic-numbers js lint rule#11008
stejskalleos merged 1 commit into
theforeman:developfrom
MariaAga:lint-magic-numbers

Conversation

@MariaAga

@MariaAga MariaAga commented May 28, 2026

Copy link
Copy Markdown
Member

Add no-magic-numbers lint rule and fix violations

Adds no-magic-numbers as a warn rule to ESLint config with common values ignored (0-8, pagination sizes, HTTP status codes, etc.).
Extracts magic numbers into named constants across ~20 files (e.g. MS_PER_SECOND, BYTES_PER_KB, TAB_WEIGHT_*, luminance coefficients).
Also adds uname to spellcheck skipWords.


This PR was created with AI assistance.

Comment thread script/lint/@theforeman/eslint-plugin-foreman/lint_generic_config.js Outdated
@stejskalleos stejskalleos self-assigned this May 29, 2026
@MariaAga
MariaAga force-pushed the lint-magic-numbers branch from 14baf3e to 0239ad7 Compare June 1, 2026 09:03
@MariaAga
MariaAga marked this pull request as ready for review June 1, 2026 09:03
@MariaAga

MariaAga commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

Thanks, updated to have

export const MS_PER_SECOND = 1000;
export const PERCENT_MULTIPLIER = 100;
export const HTTP_STATUS_CODES = { ...

Also changed the rule to be an error and not a warning so the git action will fail when magic numbers are used (for easier reviewing experience)

@MariaAga
MariaAga force-pushed the lint-magic-numbers branch from 0239ad7 to ca04d1d Compare June 1, 2026 09:06
@MariaAga MariaAga changed the title Fix magic numbers and add no-magic-numbers rule Fixes #39383 - add no-magic-numbers js lint rule Jun 1, 2026

@stejskalleos stejskalleos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few last comments; plus it needs a rebase; otherwise, LGTM.

@MariaAga

MariaAga commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Thanks @stejskalleos - resolved the comments, rebased, and updated newly added numbers to use consts

@stejskalleos
stejskalleos merged commit 6c64d50 into theforeman:develop Jul 13, 2026
62 of 67 checks passed
nadjaheitmann added a commit to ATIX-AG/foreman_snapshot_management that referenced this pull request Jul 13, 2026
The Foreman linting requires constants to be declared instead of
directly assigning numbers to variables. Otherwise, it will through the
'no-magic-numbers' error.

Corresponding Foreman core PR:

theforeman/foreman#11008

Co-Authored-By: OpenAI Codex
nadjaheitmann added a commit to ATIX-AG/foreman_snapshot_management that referenced this pull request Jul 13, 2026
The Foreman linting requires constants to be declared instead of
directly assigning numbers to variables. Otherwise, it will through the
'no-magic-numbers' error.

Corresponding Foreman core PR:

theforeman/foreman#11008

Co-Authored-By: OpenAI Codex
@Odilhao

Odilhao commented Jul 13, 2026

Copy link
Copy Markdown
Member

Looks like this made theforeman/foreman_rh_cloud#1231 fail because rh-cloud tests are not pinned to a particular version of foreman.

@MariaAga

Copy link
Copy Markdown
Member Author

@Odilhao I'll get to fixing the plugins, or plugins contributors can fix the errors themselves. There are 2 more PRs that effect plugin lint that are awaiting review/merge

nadjaheitmann added a commit to ATIX-AG/foreman_resource_quota that referenced this pull request Jul 14, 2026
The Foreman linting requires constants to be declared instead of
directly assigning numbers to variables. Otherwise, it will through the
'no-magic-numbers' error.

Corresponding Foreman core PR:

theforeman/foreman#11008

Co-Authored-By: OpenAI Codex
nadjaheitmann added a commit to ATIX-AG/foreman_resource_quota that referenced this pull request Jul 14, 2026
The Foreman linting requires constants to be declared instead of
directly assigning numbers to variables. Otherwise, it will through the
'no-magic-numbers' error.

Corresponding Foreman core PR:

theforeman/foreman#11008

Co-Authored-By: OpenAI Codex
@jeremylenz

Copy link
Copy Markdown
Contributor

There are 2 more PRs that effect plugin lint that are awaiting review/merge

which PRs are those?

@MariaAga

Copy link
Copy Markdown
Member Author

#11098 #10968
@jeremylenz

jeremylenz added a commit to theforeman/foreman_rh_cloud that referenced this pull request Jul 15, 2026
* Fix no-magic-numbers lint errors

Foreman core theforeman/foreman#11008 added the no-magic-numbers
ESLint rule to the shared lint config used to lint plugin JS via
`npm run lint:plugins`. Extract the affected numeric literals into
named constants so foreman_rh_cloud's plugin lint check passes again.

* Address review: dedupe SECONDS_PER_MINUTE/PERCENT_COMPLETE, document delays

- Extract SECONDS_PER_MINUTE and PERCENT_COMPLETE into a shared
  TaskConstants module used by both TaskHistory and TaskProgress,
  instead of duplicating the constants in each file.
- Add short comments explaining why the polling interval and form
  submit delay use their specific values, to make them easier to tune.
nadjaheitmann added a commit to theforeman/foreman_puppet that referenced this pull request Jul 17, 2026
The Foreman linting requires constants to be declared instead of
directly assigning numbers to variables. Otherwise, it will through the
'no-magic-numbers' error.

Corresponding Foreman core PR:

theforeman/foreman#11008

Co-Authored-By: OpenAI Codex
MariaAga pushed a commit to theforeman/foreman_puppet that referenced this pull request Jul 17, 2026
The Foreman linting requires constants to be declared instead of
directly assigning numbers to variables. Otherwise, it will through the
'no-magic-numbers' error.

Corresponding Foreman core PR:

theforeman/foreman#11008

Co-Authored-By: OpenAI Codex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants