Skip to content

Swift: Correct a couple of FilePath models. #14682

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

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Nov 3, 2023

Very minor fixes for the FilePath model.

The existing tests for this are unaffected.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Nov 3, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner November 3, 2023 15:35
@geoffw0 geoffw0 requested a review from rdmarsh2 November 6, 2023 08:45
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

Would it be possible to ... add a test that shows the difference?

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 6, 2023

Would it be possible to ... add a test that shows the difference?

Will do.

Note that I don't in general write tests for every single MAD source or sink - particularly when there are many similar ones, I feel that testing one of a set of near identical rows is often sufficient.

@MathiasVP
Copy link
Contributor

Note that I don't in general write tests for every single MAD source or sink - particularly when there are many similar ones, I feel that testing one of a set of near identical rows is often sufficient.

Good point. I just think it would be valuable as a justification for why this rewrite was necessary.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Nov 6, 2023

Tests added.

Copy link
Contributor

@MathiasVP MathiasVP 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 🙇!

@MathiasVP MathiasVP merged commit 84594e6 into github:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants