Hey Nick,
I think the best solution is really to find a way to only apply
certain rules to code modified after a certain date. I also don't
think it would be that hard to implement because git can output
per-line information about modification times. So you'd just run the
scalastyle rules and then if you saw errors from rules with a special
"if modified since" property, we'd only fail the line has been
modified after that date. That would even work for imports as well,
you'd just have a thing where if anyone modified some imports they
would have to fix all the imports in that file. It's at least worth a
try.
Overall I think that's the only real solution here. Doing things
closer to releases reduces the overhead of backporting, but overall
it's still going to be a very high overhead.
- Patrick
On Mon, Oct 13, 2014 at 5:46 AM, Nicholas Chammas
<nicholas.cham...@gmail.com> wrote:
> The arguments against large scale refactorings make sense. Doing them, if at
> all, during QA cycles or around releases sounds like a promising idea.
>
> Coupled with that, would it be useful to implement new rules outside of
> these potential windows for refactoring in such a way that they report on
> style errors without failing the build?
>
> That way they work like a nag and encourage developers to fix style problems
> in the course of working on their original patch. Coupled with a clear
> policy to fix style only where code is being changed anyway, this could be a
> helpful way to steadily fix problems related to new rules over time. Then,
> when we get close to the "finish line", we can make a final patch to fix the
> remaining issues for a given rule and enforce it as part of the build.
> Having the style report should also make it easier for committers to review
> style. We will have to do some work to show reports on new rules in a
> digestible way, as they will probably be very large at first, but I think
> that's a tractable problem.
>
> What do committers/reviewers think of that?
>
> As an aside, enforcing new style rules for new files only is an interesting
> idea, but you'd have to track that a file was added after the new rules were
> enforced. Otherwise bad style will be allowed after the initial checkin of
> that file. Also, enforcing style rules on changes may work in some (e.g.
> space required before "{") but is impossible in other (import ordering)
> cases, as Reynold pointed out.
>
> Nick
>
>
> 2014년 10월 13일 월요일, Matei Zaharia<matei.zaha...@gmail.com>님이 작성한 메시지:
>
>> I'm also against these huge reformattings. They slow down development and
>> backporting for trivial reasons. Let's not do that at this point, the style
>> of the current code is quite consistent and we have plenty of other things
>> to worry about. Instead, what you can do is as you edit a file when you're
>> working on a feature, fix up style issues you see. Or, as Josh suggested,
>> some way to make this apply only to new files would help.
>>
>> Matei
>>
>> On Oct 12, 2014, at 10:16 PM, Patrick Wendell <pwend...@gmail.com> wrote:
>>
>> > Another big problem with these patches are that they make it almost
>> > impossible to backport changes to older branches cleanly (there
>> > becomes like 100% chance of a merge conflict).
>> >
>> > One proposal is to do this:
>> > 1. We only consider new style rules at the end of a release cycle,
>> > when there is the smallest chance of wanting to backport stuff.
>> > 2. We require that they are submitted in individual patches with a (a)
>> > new style rule and (b) the associated changes. Then we can also
>> > evaluate on a case-by-case basis how large the change is for each
>> > rule. For rules that require sweeping changes across the codebase,
>> > personally I'd vote against them. For rules like import ordering that
>> > won't cause too much pain on the diff (it's pretty easy to deal with
>> > those conflicts) I'd be okay with it.
>> >
>> > If we went with this, we'd also have to warn people that we might not
>> > accept new style rules if they are too costly to enforce. I'm guessing
>> > people will still contribute even with those expectations.
>> >
>> > - Patrick
>> >
>> > On Sun, Oct 12, 2014 at 9:39 PM, Reynold Xin <r...@databricks.com>
>> > wrote:
>> >> I actually think we should just take the bite and follow through with
>> >> the
>> >> reformatting. Many rules are simply not possible to enforce only on
>> >> deltas
>> >> (e.g. import ordering).
>> >>
>> >> That said, maybe there are better windows to do this, e.g. during the
>> >> QA
>> >> period.
>> >>
>> >> On Sun, Oct 12, 2014 at 9:37 PM, Josh Rosen <rosenvi...@gmail.com>
>> >> wrote:
>> >>
>> >>> There are a number of open pull requests that aim to extend Spark's
>> >>> automated style checks (see
>> >>> https://issues.apache.org/jira/browse/SPARK-3849 for an umbrella
>> >>> JIRA).
>> >>> These fixes are mostly good, but I have some concerns about merging
>> >>> these
>> >>> patches. Several of these patches make large reformatting changes in
>> >>> nearly every file of Spark, which makes it more difficult to use `git
>> >>> blame` and has the potential to introduce merge conflicts with all
>> >>> open PRs
>> >>> and all backport patches.
>> >>>
>> >>> I feel that most of the value of automated style-checks comes from
>> >>> allowing reviewers/committers to focus on the technical content of
>> >>> pull
>> >>> requests rather than their formatting. My concern is that the
>> >>> convenience
>> >>> added by these new style rules will not outweigh the other overheads
>> >>> that
>> >>> these reformatting patches will create for the committers.
>> >>>
>> >>> If possible, it would be great if we could extend the style checker to
>> >>> enforce the more stringent rules only for new code additions /
>> >>> deletions.
>> >>> If not, I don't think that we should proceed with the reformatting.
>> >>> Others
>> >>> might disagree, though, so I welcome comments / discussion.
>> >>>
>> >>> - Josh
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org
>> > For additional commands, e-mail: dev-h...@spark.apache.org
>> >
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org
>> For additional commands, e-mail: dev-h...@spark.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org
For additional commands, e-mail: dev-h...@spark.apache.org
