Skip to content

Swift: Correct a couple of FilePath models.#14682

Merged
MathiasVP merged 3 commits intogithub:mainfrom
geoffw0:filepathclosure
Nov 6, 2023
Merged

Swift: Correct a couple of FilePath models.#14682
MathiasVP merged 3 commits intogithub:mainfrom
geoffw0:filepathclosure

Conversation

@geoffw0
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

geoffw0 commented Nov 6, 2023

Tests added.

Copy link
Copy Markdown
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