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

refactor IR tests #151

Merged
merged 2 commits into from
Feb 20, 2023
Merged

refactor IR tests #151

merged 2 commits into from
Feb 20, 2023

Conversation

grjte
Copy link
Contributor

@grjte grjte commented Feb 17, 2023

This PR includes a trivial clippy fix and a quick refactor to split the IR tests into modules to make them more manageable and easier to update later. This addresses #138

Copy link
Contributor

@tohrnii tohrnii left a comment

Choose a reason for hiding this comment

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

Thank you @grjte, looks good to me. One question is if we should replace boundary_constraints and integrity_constraints in function names to their abbreviated forms (bc and ic) to be consistent except for boundary_constraints() and integrity_constraints()? If you agree with this change maybe it can be done with this PR?

@grjte
Copy link
Contributor Author

grjte commented Feb 17, 2023

Thank you @grjte, looks good to me. One question is if we should replace boundary_constraints and integrity_constraints in function names to their abbreviated forms (bc and ic) to be consistent except for boundary_constraints() and integrity_constraints()? If you agree with this change maybe it can be done with this PR?

Do you mean the accessor functions? I would probably leave them as it is and keep this PR as a tests refactor

@tohrnii
Copy link
Contributor

tohrnii commented Feb 17, 2023

No I actually meant just for test names. For eg, renaming boundary_constraints_with_constants to bc_with_constants etc.

@grjte grjte force-pushed the grjte-ir-refactor-tests branch from 059b60a to 963d91c Compare February 20, 2023 10:17
@grjte
Copy link
Contributor Author

grjte commented Feb 20, 2023

No I actually meant just for test names. For eg, renaming boundary_constraints_with_constants to bc_with_constants etc.

Ah, I see. The way I had done it was to shorten it in the error case and leave it unshortened for the main test, but I don't feel strongly. I've changed it in all cases except when there's nothing else in the name (e.g. boundary_constraints)

@grjte grjte merged commit e74af8a into next Feb 20, 2023
@grjte grjte deleted the grjte-ir-refactor-tests branch February 20, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants