add scalafmt and docs for combineWith #128
Conversation
|
(Please don't mix formatting changes with other changes - these should be
in separate commits, and ideally in different PRs if they are not related.)
Regarding formatting: this PR does not address my concerns in #127. The
number of formatted files is not the issue. The scalafmt config not
matching existing style is the issue. If scalafmt is to be used, it should
be on the whole project, and with a config that matches existing style as
much as possible.
Regarding added comments: tbh the combineWith scaladoc seems too verbose
for a method that has N copies. I should probably write a doc section about
combineWith, and then we can just link to it from the scaladoc comment.
…On Wed., Aug. 7, 2024, 3:58 a.m. Eason du, ***@***.***> wrote:
@doofin <https://github.com/doofin> requested your review on: #128
<#128> add scalafmt and docs for
combineWith as a code owner.
—
Reply to this email directly, view it on GitHub
<#128 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAECBMETA5XIHWGAKETQL43ZQH4TVAVCNFSM6AAAAABMEE5FFOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTG44TANRSGM2TCMY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
ok, but you said before "I can tolerate a small number of changes in formatting here and there for the sake of easier collaboration with scalafmt", so I just formatted a few. Without scalafmt, it's very difficult to make contrib to this project in addition, I can split this PR into format and docs. |
|
Sorry if that wasn't clear, what I meant is that it's impossible to write a scalamft config file that perfectly matches Airstream's current code style, because scalafmt is simply not configurable enough for that. So if the proposed scalafmt rules end up deviating a little bit from the current style, it's ok. I didn't set up scalafmt on my OSS projects because I don't like it. Its configuration options are too limited, so it makes the code look bad one way or another no matter what I do. I am willing to tolerate scalafmt for the sake of collaboration, but it has to be configured to match the current style as much as possible. That is not a trivial task, and I personally never had time for hunting down such a configuration. TBH I don't know if it's possible to come close enough. I hope it is. Most of all I dislike scalafmt's prescriptions regarding where to insert newlines. It often doesn't know how to do the right thing, resulting in overly verbose formatting style, and does not know how to keep things as-is. |
|
ok, then you actually refuse scalafmt. The caveat will be contribs won't know which formatting style is expected other than looking at existing code. I still hope scalafmt can be setup in the future, although not now Can you help me put the docs in right place and I'll close this PR? |
|
External contributions are quite rare, especially bigger ones, so in practice, contributors try to match the formatting style in the files that they're modifying, and if it doesn't look right to me, I'll just run my IDE's formatter on the PR and call it a day. I know, lame, but it's been working for now, absent of a suitable scalafmt config, as I mentioned. I plan to write a doc section on |
|
thanks, docs looks great! |
|
@doofin I spent today wrangling scalafmt to match my IntelliJ formatter config. It's not done yet, there are some issues remaining, but it looks promising, dare I say. I'll finish it eventually, and will apply it to all my OSS projects. Started here: raquo/Laminar#167 |
continue from #127
I added some docs for the dynamic semantics of combineWith