[
https://issues.apache.org/jira/browse/SAMOA-6?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14303464#comment-14303464
]
Matthieu Morel commented on SAMOA-6:
------------------------------------
We've had some hickups with the github integration. Let me reproduce the
conversation from github here since it was not copied to the jira:
gdfm commented a day ago
The patch is too big for GitHub, so I checked it out manually.
There is some whitespace before an EOL that can be removed. For example, in in
the header of eclipse-format.xml.
The header of all the xml files (pom.xml in samoa-api, samoa-instances,
log4j.xml, etc..) was changed. We shouldn't reformat the header.
I am not sure about some of the changes, e.g.;
* @return true if the EntranceProcessor is ready to provide the next event,
false otherwise.
* @return true if the EntranceProcessor is ready to provide the next event,
* false otherwise. */ Do we need to wrap here? And why is there some space at
the beginning of the second line?
In AMRulesRegressorProcessor.java there is a piece of commented pseudocode to
express the algorithm that serves as documentation. This patch makes it very
hard to read. Can we avoid that somehow?
The same happens in LocalStatisticsProcessor. Can we avoid reformatting those
pieces?
Some reformatting is pretty weird. E.g.,
learners/classifiers/rules/common/Rule.java has a comment that is split in one
word per line.
There are some strange EOL characters in
BinaryTreeNumericAttributeClassObserverRegression.java
Once these small details are fixed I'm +1.
I'd also like to have another +1 from somebody else if possible.
matthieumorel added some commits 23 hours ago
Matthieu Morel Formatting updates … 11b716f
Matthieu Morel Fix headers in xml files 73a6824
Matthieu Morel Format documentation with new formatting settings 5543ecd
Matthieu Morel Fix line comments indentation b79e92b
Matthieu Morel
matthieumorel commented 22 hours ago
Thanks for taking a close look at this. I've updated the patch accordingly, mod
some comments inline:
There is some whitespace before an EOL that can be removed. For example, in in
the header of eclipse-format.xml.
This is added automatically before the line break to respect the space before
the indentation in the next line. I think it's ok to keep that.
I formatted the xml files because some had weird indentation, but the idea is
that it's just more readable.
The header of all the xml files (pom.xml in samoa-api, samoa-instances,
log4j.xml, etc..) was changed. We shouldn't reformat the header.
Right, fixed.
I am not sure about some of the changes, e.g.;
* @return true if the EntranceProcessor is ready to provide the next event,
false otherwise.
* @return true if the EntranceProcessor is ready to provide the next event,
* false otherwise. */ Do we need to wrap here? And why is there some space at
the beginning of the second line?
This is because text in comments currently wraps at 80. I changed to 120.
In AMRulesRegressorProcessor.java there is a piece of commented pseudocode to
express the algorithm that serves as documentation. This patch makes it very
hard to read. Can we avoid that somehow?
Can we avoid reformatting those pieces?
Simplest way is to disable formatting for block comments (starting with /*) and
rewrite the pseudo-code
The same happens in LocalStatisticsProcessor.
I don't see what the problem is with formatting comments in that class. There
is a piece of code that is commented but it looks like it's unused code rather
than documentation.
Some reformatting is pretty weird. E.g.,
learners/classifiers/rules/common/Rule.java has a comment that is split in one
word per line.
I've manually reformatted those lines and disabled formatting for line comments
There are some strange EOL characters in
BinaryTreeNumericAttributeClassObserverRegression.java
Which ones?
Gianmarco De Francisci Morales
gdfm commented an hour ago
Looks good now, apart from the last point.
The strange EOLs are the following ones (in
samoa/moa/classifiers/core/attributeclassobservers/BinaryTreeNumericAttributeClassObserverRegression.java):
/**
- * Class for observing the class data distribution for a numeric attribute
using a binary tree.
- * This observer monitors the class distribution of a given attribute.
+ * Class for observing the class data distribution for a numeric attribute
using a binary tree. This observer monitors^M
+ * the class distribution of a given attribute.^M
*
- * <p>Learning Adaptive Model Rules from High-Speed Data Streams, ECML 2013,
E. Almeida, C. Ferreira, P. Kosina and J. Gama; </p>
+ * <p>^M
+ * Learning Adaptive Model Rules from High-Speed Data Streams, ECML 2013, E.
Almeida, C. Ferreira, P. Kosina and J.^M
+ * Gama;^M
+ * </p>^M
> Reformat codebase and add formatting guidelines
> -----------------------------------------------
>
> Key: SAMOA-6
> URL: https://issues.apache.org/jira/browse/SAMOA-6
> Project: SAMOA
> Issue Type: Task
> Components: SAMOA-API, SAMOA-Instances, SAMOA-Local, SAMOA-S4,
> SAMOA-Samza, SAMOA-Storm, SAMOA-Threads
> Reporter: Gianmarco De Francisci Morales
>
> Migrating issue 118 from GitHub https://github.com/yahoo/samoa/issues/118
> We need to agree on formatting guidelines, write them down, and reformat the
> whole codebase in a single commit.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
