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

Add number of items based record multiline formatter #1169

Merged

Conversation

Fizzixnerd
Copy link
Contributor

See #1143.

I originally wrote the bulk of the code back when I was still calling it "LogicalSize". I have gone through the diffs looking to make sure I removed all references to that, but just be aware if I missed one that I'm not trying to sneak it in!

Currently the tests assume the current behaviour discussed in #1167. If that changes, then one of the tests here will need to change as well. I have affixed a FIXME to the test in question that can be removed when that issue is closed, but please let me know if you want it done a different way.

@nojaf
Copy link
Contributor

nojaf commented Sep 26, 2020

@Fizzixnerd I'll review this thoroughly another time but I already want to point out that records have a broader scope.
At first glance, I didn't see any tests for signature files and nothing related to object expressions.
Please consider test-driven development for this project, it can help in not introducing changes without test coverage.

@Fizzixnerd
Copy link
Contributor Author

@Fizzixnerd I'll review this thoroughly another time but I already want to point out that records have a broader scope.
At first glance, I didn't see any tests for signature files and nothing related to object expressions.
Please consider test-driven development for this project, it can help in not introducing changes without test coverage.

Thanks @nojaf.

Yes, I am sort of winging it with what needs to be changed. I don't think I've ever explicitly used signature files or object expressions before, so it didn't occur to me they would be affected. I will write tests first for this project from now on, but if I'm not aware of everything that could be affected I'm afraid it won't be enough. Is there an issue/PR in the past that touched all aspects of records that I could use as a reference? I obviously don't want to miss anything else.

@nojaf
Copy link
Contributor

nojaf commented Sep 26, 2020

I don't think I've ever explicitly used signature files or object expressions

Yeah, I know I'm asking more than you might need yourself but it is to remain consistent.
I would take a look at MultilineBlockBracketsOnSameColumnRecordTests.fs.

You can write tests for signature files by using formatSourceString true.

@nojaf
Copy link
Contributor

nojaf commented Sep 28, 2020

Great PR @Fizzixnerd! And thanks for reporting the inconsistencies you found along the way.

@nojaf nojaf merged commit 746514d into fsprojects:master Sep 28, 2020
@Fizzixnerd
Copy link
Contributor Author

No problem, thanks for fixing those nitpicks.

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