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

Hide data in Domain #1277

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Hide data in Domain #1277

wants to merge 6 commits into from

Conversation

btalamini
Copy link
Collaborator

@btalamini btalamini commented Nov 29, 2024

Following up on Mike's comment: this changes Domain from a struct to a class, with the expected hiding of internal data.

Copy link
Collaborator

@tupek2 tupek2 left a comment

Choose a reason for hiding this comment

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

Looks good to me, seems like we need to get that new formatting in. Its going crazy on these files.

@btalamini
Copy link
Collaborator Author

Looks good to me, seems like we need to get that new formatting in. Its going crazy on these files.

I agree. In particular, the forced alignment of the right side of = assignments triggers changes to lines that weren't touched by the PR, making the PR bigger, and more difficult to review, since you have to read closely to see if the change is substantial or style.

@btalamini
Copy link
Collaborator Author

Let's hold this back until I get the boundary condition PR in. I'll handle the conflicts that emerge in this PR later.

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

Successfully merging this pull request may close these issues.

3 participants