Closed doofin closed 3 months ago
(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 https://github.com/raquo/Airstream/pull/128 add scalafmt and docs for combineWith as a code owner.
— Reply to this email directly, view it on GitHub https://github.com/raquo/Airstream/pull/128#event-13790623513, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAECBMETA5XIHWGAKETQL43ZQH4TVAVCNFSM6AAAAABMEE5FFOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTG44TANRSGM2TCMY . You are receiving this because your review was requested.Message ID: @.***>
Yes, this PR has +10K - 5K line of code changes, almost all of them are undesirable to me. I can tolerate a small number of changes in formatting here and there for the sake of easier collaboration with scalafmt, but I don't want to reformat the whole codebase in some other style, just because it happens to be the default scalafmt preset. Aside from my formatting preferences, massive changes like this also hide the real git history (I mean when using
git blame
) which is important to me when dealing with complex issues.
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 combineWith
in the next few days, then you can link to it. I'll ping you here when it's done.
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: https://github.com/raquo/Laminar/pull/167
continue from https://github.com/raquo/Airstream/pull/127
I added some docs for the dynamic semantics of combineWith