Skip to content
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

prefer-math-min-max issue with luxon objects #2562

Open
eldair opened this issue Feb 18, 2025 · 3 comments
Open

prefer-math-min-max issue with luxon objects #2562

eldair opened this issue Feb 18, 2025 · 3 comments
Labels

Comments

@eldair
Copy link

eldair commented Feb 18, 2025

Prefer math min max should not be reported for luxon objects because Math.min/max returns timestamp instead of original object which causes errors if not corrected and also there is loss of other information if object is recreated from Math result (like timezone)

prefer-math-min-max

let start = DateTime.local().startOf('day').minus({days: 5});
let end = DateTime.local().startOf('day').minus({days: 1});

let min = Math.min(start, end);
// min is now a timestamp and not an object.
@eldair eldair added the bug label Feb 18, 2025
@github-actions github-actions bot changed the title prefer-math-min-max issue with luxon objects prefer-math-min-max issue with luxon objects Feb 18, 2025
@axetroy
Copy link
Contributor

axetroy commented Feb 20, 2025

Since there is no Type-aware in the Plugin, we cannot know its type

This seems normal to me, using implicit valueOf() is not the recommended way

let start = DateTime.local().startOf('day').minus({days: 5});
let end = DateTime.local().startOf('day').minus({days: 1});

// ❌ implicit `valueOf()`
let min = start > end ? end : start;

You should write to this

let start = DateTime.local().startOf('day').minus({days: 5});
let end = DateTime.local().startOf('day').minus({days: 1});

// ✅
let min = start.valueOf() > end.valueOf() ? end : start;

@eldair
Copy link
Author

eldair commented Feb 20, 2025

@axetroy thanks for getting back to me.
I actually like your suggestion but that line will not report I have used implicit valueOf, only math errors.

Maybe autofix should be turned off for anything except numbers to Math.min/max, the way it works seems unsafe.

@eldair
Copy link
Author

eldair commented Feb 20, 2025

If disabling auto fix for non numbers is something you would consider I could make a pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants