Re: [GitHub] jmeter issue #358: Checkstyle: LineLength max 165, AnonInnerLength 45 and ot...
Sorry, for the confusing email formatting. Of course, the way it is
displayed in the mail does not reflect the original source code
formatting, but i think you catch my point: i'm not very happy with
breaking up lines that are only 120 chars long..
Cheers
Peter
2017-12-21 10:37 GMT+01:00 Peter Doornbosch :
> I wonder why you break up lines like these
>
> useBrowserCompatibleMultipartMode = new
> JCheckBox(JMeterUtils.getResString("use_multipart_mode_browser")); //
> $NON-NLS-1$
>
> into
>
> useBrowserCompatibleMultipartMode =
> new
> JCheckBox(JMeterUtils.getResString("use_multipart_mode_browser")); //
> $NON-NLS-1$
>
> IMHO this does not improve readability, but makes it worse.
>
> Moreover, the description says something about "max line length 165",
> the line above is only 120 chars.
>
> I admit max line length is also a matter of personal style, but i
> couldn't find any docs about what JMeter prescribes in this matter.
> Given no official rules, i would be careful with changes that won't be
> seen as improvements by all of us (your PR does contain a lot of
> changes that _are_ evident improvements).
>
> My 2cts.
>
> Peter
>
> 2017-12-21 8:33 GMT+01:00 ham1 :
>> Github user ham1 commented on the issue:
>>
>> https://github.com/apache/jmeter/pull/358
>>
>> Sorry, yeah I see how this is difficult to review. I tend to change
>> things as I see them in the file I'm editing. I will revise and split it up
>> into one type of change per commit, is that ok? It might be more than 10
>> files but should be easier to review.
>>
>>
>> ---
Re: [GitHub] jmeter issue #358: Checkstyle: LineLength max 165, AnonInnerLength 45 and ot...
I wonder why you break up lines like these
useBrowserCompatibleMultipartMode = new
JCheckBox(JMeterUtils.getResString("use_multipart_mode_browser")); //
$NON-NLS-1$
into
useBrowserCompatibleMultipartMode =
new
JCheckBox(JMeterUtils.getResString("use_multipart_mode_browser")); //
$NON-NLS-1$
IMHO this does not improve readability, but makes it worse.
Moreover, the description says something about "max line length 165",
the line above is only 120 chars.
I admit max line length is also a matter of personal style, but i
couldn't find any docs about what JMeter prescribes in this matter.
Given no official rules, i would be careful with changes that won't be
seen as improvements by all of us (your PR does contain a lot of
changes that _are_ evident improvements).
My 2cts.
Peter
2017-12-21 8:33 GMT+01:00 ham1 :
> Github user ham1 commented on the issue:
>
> https://github.com/apache/jmeter/pull/358
>
> Sorry, yeah I see how this is difficult to review. I tend to change
> things as I see them in the file I'm editing. I will revise and split it up
> into one type of change per commit, is that ok? It might be more than 10
> files but should be easier to review.
>
>
> ---
[GitHub] jmeter issue #358: Checkstyle: LineLength max 165, AnonInnerLength 45 and ot...
Github user ham1 commented on the issue: https://github.com/apache/jmeter/pull/358 Sorry, yeah I see how this is difficult to review. I tend to change things as I see them in the file I'm editing. I will revise and split it up into one type of change per commit, is that ok? It might be more than 10 files but should be easier to review. ---
[GitHub] jmeter issue #358: Checkstyle: LineLength max 165, AnonInnerLength 45 and ot...
Github user pmouawad commented on the issue: https://github.com/apache/jmeter/pull/358 Hi @ham1 , Thanks for this PR. Unfortunately I am not very comfortable with PR that touch a lot of file and where the code modification may touch more things than what the PR describes. Would it be possible to rebase your PR and split it into at max 10 files ? It is easier to review and merging is faster for me, which will mean I'll merge it earlier and you won't have to rebase. Also if possible, try to make the PR really only touch what it pretends to. Sonar fixes might sometimes break existing code so they need careful review and merge. Thanks a lot again for all your work ! Regards ---
[GitHub] jmeter issue #358: Checkstyle: LineLength max 165, AnonInnerLength 45 and ot...
Github user codecov-io commented on the issue: https://github.com/apache/jmeter/pull/358 # [Codecov](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=h1) Report > Merging [#358](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=desc) into [trunk](https://codecov.io/gh/apache/jmeter/commit/427907f9d2ca45f8077acf9398a140834d75e3bd?src=pr&el=desc) will **decrease** coverage by `<.01%`. > The diff coverage is `42.42%`. [](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=tree) ```diff @@ Coverage Diff @@ ## trunk #358 +/- ## - Coverage 58.19% 58.19% -0.01% - Complexity1021010211 +1 Files 1161 1161 Lines 7418974262 +73 Branches 7322 7324 +2 + Hits 4317443215 +41 - Misses2851728549 +32 Partials 2498 2498 ``` | [Impacted Files](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=tree) | Coverage Π| Complexity Π| | |---|---|---|---| | [...protocol/http/sampler/LazySchemeSocketFactory.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9zYW1wbGVyL0xhenlTY2hlbWVTb2NrZXRGYWN0b3J5LmphdmE=) | `66.66% <ø> (ø)` | `5 <0> (ø)` | :arrow_down: | | [...jmeter/protocol/http/util/accesslog/LogFilter.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC91dGlsL2FjY2Vzc2xvZy9Mb2dGaWx0ZXIuamF2YQ==) | `81.3% <ø> (ø)` | `35 <0> (ø)` | :arrow_down: | | [.../org/apache/jmeter/report/core/SampleMetadata.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvcmVwb3J0L2NvcmUvU2FtcGxlTWV0YWRhdGEuamF2YQ==) | `26.08% <ø> (ø)` | `8 <0> (ø)` | :arrow_down: | | [...e/org/apache/jmeter/gui/action/OpenLinkAction.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL2NvcmUvb3JnL2FwYWNoZS9qbWV0ZXIvZ3VpL2FjdGlvbi9PcGVuTGlua0FjdGlvbi5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...meter/protocol/mail/sampler/MailReaderSampler.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL21haWwvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvbWFpbC9zYW1wbGVyL01haWxSZWFkZXJTYW1wbGVyLmphdmE=) | `16.39% <0%> (ø)` | `22 <0> (ø)` | :arrow_down: | | [...ocol/http/sampler/HttpClientDefaultParameters.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9zYW1wbGVyL0h0dHBDbGllbnREZWZhdWx0UGFyYW1ldGVycy5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...ter/protocol/http/sampler/ResourcesDownloader.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9zYW1wbGVyL1Jlc291cmNlc0Rvd25sb2FkZXIuamF2YQ==) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...rotocol/jms/sampler/render/MapMessageRenderer.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2ptcy9vcmcvYXBhY2hlL2ptZXRlci9wcm90b2NvbC9qbXMvc2FtcGxlci9yZW5kZXIvTWFwTWVzc2FnZVJlbmRlcmVyLmphdmE=) | `11.53% <0%> (ø)` | `1 <0> (ø)` | :arrow_down: | | [...rc/jorphan/org/apache/jorphan/util/HeapDumper.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL2pvcnBoYW4vb3JnL2FwYWNoZS9qb3JwaGFuL3V0aWwvSGVhcER1bXBlci5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: | | [...p/org/apache/jmeter/protocol/http/proxy/Proxy.java](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree#diff-c3JjL3Byb3RvY29sL2h0dHAvb3JnL2FwYWNoZS9qbWV0ZXIvcHJvdG9jb2wvaHR0cC9wcm94eS9Qcm94eS5qYXZh) | `0% <0%> (ø)` | `0 <0> (ø)` | :arrow_down: | | ... and [37 more](https://codecov.io/gh/apache/jmeter/pull/358/diff?src=pr&el=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Π= absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=footer). Last update [427907f...ec91dd9](https://codecov.io/gh/apache/jmeter/pull/358?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
