FSchumacher commented on a change in pull request #571:
URL: https://github.com/apache/jmeter/pull/571#discussion_r432845144
##########
File path: xdocs/usermanual/component_reference.xml
##########
@@ -6720,13 +6719,34 @@ Both Chrome and Internet Explorer use the same trust
store for certificates.
By default it also removes <code>If-Modified-Since</code> and
<code>If-None-Match</code> headers.
These are used to determine if the browser cache items are up to date;
when recording one normally wants to download all the content.
- To change which additional headers are removed, define the JMeter
property <code>proxy.headers.remove</code>
+ To change which additional headers are removed, define the JMeter
property <code>proxy.heade'rs.remove</code>
Review comment:
Seems to be a strange name for a property. Is `proxy.heade'rs` really
correct?
##########
File path:
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/gui/ProxyControlGui.java
##########
@@ -442,6 +483,12 @@ public void actionPerformed(ActionEvent action) {
}
} else if (command.equals(ACTION_RESTART)) {
model.stopProxy();
+ // what is the last number for numbering samplers, ex : 96
+ int iRequestNumber = model.getRequestNumberFromSamplerCreator();
Review comment:
If the *`i`* at the beginning of `iRequestNumber` is used to indicate an
integer, I would remove it and name the variable `requestNumber`. That is the
way variables are named in JMeter most of the time. Most developers around
JMeter will probably use an IDE, which shows the type of the variable anyway.
##########
File path: xdocs/usermanual/component_reference.xml
##########
@@ -6720,13 +6719,34 @@ Both Chrome and Internet Explorer use the same trust
store for certificates.
By default it also removes <code>If-Modified-Since</code> and
<code>If-None-Match</code> headers.
These are used to determine if the browser cache items are up to date;
when recording one normally wants to download all the content.
- To change which additional headers are removed, define the JMeter
property <code>proxy.headers.remove</code>
+ To change which additional headers are removed, define the JMeter
property <code>proxy.heade'rs.remove</code>
as a comma-separated list of headers.
</property>
<property name="Add Assertions" required="Yes">Add a blank assertion
to each sampler?</property>
<property name="Regex Matching" required="Yes">Use Regex Matching when
replacing variables? If checked replacement will use word boundaries, i.e. it
will only replace word matching values of variable, not part of a word. A word
boundary follows Perl5 definition and is equivalent to <code>\b</code>. More
information below in the paragraph about "<code>User Defined Variable
replacement</code>".</property>
<property name="Prefix/Transaction name" required="No">Add a prefix to
sampler name during recording (Prefix mode). Or replace sampler name by user
chosen name (Transaction name)</property>
- <property name="Create new transaction after request (ms)">Inactivity
time between two requests needed to consider them in two separate
groups.</property>
+ <property name="Numbering Sampler Choice" required="Yes">Select
the numbering mode for sampler name<br/>
Review comment:
Looks like a mismatch in spaces at the front. I think the `property`
tags should match up.
##########
File path: bin/jmeter.properties
##########
@@ -604,8 +604,15 @@ upgrade_properties=/bin/upgrade.properties
# it assumes that the user has clicked a new URL
#proxy.pause=5000
-# Add numeric suffix to Sampler names (default true)
+# Add numeric to Sampler names (default true)
Review comment:
Some placeholder has to be used (in my opinion) instead of *suffix* or
use *number* instead of *numeric*.
##########
File path:
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/AbstractSamplerCreator.java
##########
@@ -86,30 +75,49 @@ public AbstractSamplerCreator() {
/**
* @return int request number
*/
- protected static int getRequestNumber() {
+ public int getRequestNumber() {
Review comment:
Why did you change it to be non-static? (I am not sure, I like the
static, but I would like to here your reasoning.
##########
File path: xdocs/usermanual/component_reference.xml
##########
@@ -6720,13 +6719,34 @@ Both Chrome and Internet Explorer use the same trust
store for certificates.
By default it also removes <code>If-Modified-Since</code> and
<code>If-None-Match</code> headers.
These are used to determine if the browser cache items are up to date;
when recording one normally wants to download all the content.
- To change which additional headers are removed, define the JMeter
property <code>proxy.headers.remove</code>
+ To change which additional headers are removed, define the JMeter
property <code>proxy.heade'rs.remove</code>
as a comma-separated list of headers.
</property>
<property name="Add Assertions" required="Yes">Add a blank assertion
to each sampler?</property>
<property name="Regex Matching" required="Yes">Use Regex Matching when
replacing variables? If checked replacement will use word boundaries, i.e. it
will only replace word matching values of variable, not part of a word. A word
boundary follows Perl5 definition and is equivalent to <code>\b</code>. More
information below in the paragraph about "<code>User Defined Variable
replacement</code>".</property>
<property name="Prefix/Transaction name" required="No">Add a prefix to
sampler name during recording (Prefix mode). Or replace sampler name by user
chosen name (Transaction name)</property>
- <property name="Create new transaction after request (ms)">Inactivity
time between two requests needed to consider them in two separate
groups.</property>
+ <property name="Numbering Sampler Choice" required="Yes">Select
the numbering mode for sampler name<br/>
+ <ul>
+ <li><code>Without numbering</code> no numbering, ex :
'/a.png'</li>
Review comment:
*ex : ...* is probably a French abbreviation (it means *excercise* in
English). In English you can use *e.g.*, or *for example*. Also note, that
there is no space before a colon in English typography. And regarding the
markup of `/a.png`, I would use a `code` block.
##########
File path:
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/DefaultSamplerCreator.java
##########
@@ -283,32 +285,58 @@ public boolean isErrorDetected() {
* @param sampler {@link HTTPSamplerBase}
* @param request {@link HttpRequestHdr}
*/
- protected void computeSamplerName(HTTPSamplerBase sampler,
- HttpRequestHdr request) {
- String prefix = request.getPrefix();
+ protected void computeSamplerName(HTTPSamplerBase sampler, HttpRequestHdr
request) {
+ String prefix = request.getPrefix(); // ppp
Review comment:
What is the meaning of *ppp* ?
##########
File path:
src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/proxy/DefaultSamplerCreator.java
##########
@@ -283,32 +285,58 @@ public boolean isErrorDetected() {
* @param sampler {@link HTTPSamplerBase}
* @param request {@link HttpRequestHdr}
*/
- protected void computeSamplerName(HTTPSamplerBase sampler,
- HttpRequestHdr request) {
- String prefix = request.getPrefix();
+ protected void computeSamplerName(HTTPSamplerBase sampler, HttpRequestHdr
request) {
+ String prefix = request.getPrefix(); // ppp
int httpSampleNameMode = request.getHttpSampleNameMode();
if (!HTTPConstants.CONNECT.equals(request.getMethod()) &&
isNumberRequests()) {
if(StringUtils.isNotEmpty(prefix)) {
+ // with a prefix name
Review comment:
Can these parts be extracted in smaller functions/methods? You could use
names to tell the reader what the function is doing instead of using a comment
and at the same time the code gets less (at least here :) )
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
