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

Sort hash maps in Settings display #10370

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Sort hash maps in Settings display #10370

merged 1 commit into from
Mar 12, 2024

Conversation

charliermarsh
Copy link
Member

Summary

We had a report of a test failure on a specific architecture, and looking into it, I think the test assumes that the hash keys are iterated in a specific order. This PR thus adds a variant to our settings display macro specifically for maps and sets. Like CacheKey, it sorts the keys when printing.

Closes #10359.

@charliermarsh charliermarsh added the bug Something isn't working label Mar 12, 2024
seaborn = sns,
tensorflow = tf,
tkinter = tk,
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also now more consistent with our array printing.

pub classes: FxHashSet<String>,
pub constants: FxHashSet<String>,
pub variables: FxHashSet<String>,
pub no_lines_before: FxHashSet<ImportSection>,
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed these to use Fx for consistency with other settings, now that we sort in display anyway.

@charliermarsh charliermarsh force-pushed the charlie/set branch 3 times, most recently from 8de82c6 to 667cb9b Compare March 12, 2024 19:26
Copy link
Contributor

github-actions bot commented Mar 12, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh
Copy link
Member Author

I think the ecosystem check is a false positive? I see that violation on main and on this branch.

@charliermarsh charliermarsh merged commit c56fb6e into main Mar 12, 2024
17 checks passed
@charliermarsh charliermarsh deleted the charlie/set branch March 12, 2024 19:59
Comment on lines +162 to +172
write!($fmt, "{}{} = ", $prefix, stringify!($field))?;
if $settings.$field.is_empty() {
writeln!($fmt, "{{}}")?;
} else {
writeln!($fmt, "{{")?;
for (key, value) in $settings.$field.iter().sorted_by(|(left, _), (right, _)| left.cmp(right)) {
writeln!($fmt, "\t{key} = {value},")?;
}
writeln!($fmt, "}}")?;
}
}
Copy link
Member

@MichaReiser MichaReiser Mar 18, 2024

Choose a reason for hiding this comment

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

Nit: I think it would be good to move out as much code as possible from the macro. we could possibly do this by having a DisplaySet and DisplayMap struct that we would instantiate here (and call into) rather than implementing the logic inside of the macro. This has the added benefit that the logic can be reused elsewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test display_default_settings is failing on 32 bits archs
3 participants