samoa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthieu Morel (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SAMOA-6) Reformat codebase and add formatting guidelines
Date Tue, 03 Feb 2015 15:43:35 GMT

    [ 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)

Mime
View raw message