RAT-355 and RAT-366 fixes in one package #233
Conversation
src/site/apt/index.apt.vm
Outdated
|
|
||
| * Add a new {{{./matcher_def.html}Matcher definition}}. Requires Java expertice. | ||
|
|
||
| * Add a new definition format. Requires java expertice as well as expertice with the format. |
There was a problem hiding this comment.
Typo 2x: expertise;
I'd prefer to uppercase Java.
src/site/apt/license_def.apt.vm
Outdated
| How to define licenses in Apache Rat | ||
|
|
||
| All licenses in Apache Rat are defined in configuration files. There is a default | ||
| {{{https://github.com/apache/creadur-rat/blob/master/apache-rat-core/src/main/resources/org/apache/rat/default.xml} |
There was a problem hiding this comment.
src/site/apt/license_def.apt.vm
Outdated
|
|
||
| All licenses in Apache Rat are defined in configuration files. There is a default | ||
| {{{https://github.com/apache/creadur-rat/blob/master/apache-rat-core/src/main/resources/org/apache/rat/default.xml} | ||
| XML based configuration file}} that can server as a good example of various definitions. |
src/site/apt/license_def.apt.vm
Outdated
| {{{https://github.com/apache/creadur-rat/blob/master/apache-rat-core/src/main/resources/org/apache/rat/default.xml} | ||
| XML based configuration file}} that can server as a good example of various definitions. | ||
|
|
||
| It is possible to create new parsers for different configuration formats. But that task is beyone the |
src/site/apt/license_def.apt.vm
Outdated
|
|
||
| * Families | ||
|
|
||
| Families are groups that define licenses that share similarities. Each family has an id and a name, and example |
There was a problem hiding this comment.
two sentences ? Each family has ..... An example .....
src/site/apt/license_def.apt.vm
Outdated
| included document. It has to exist by the time all the configuration files are read. The "id" element | ||
| is optional, if it is not provided the id will be set to the id of the associated family. However, the | ||
| "id" property must be unique across all licenses. The "name" property is also optional. If not specified | ||
| the name of the associated family will be used. It is recommened that each license have a unique name |
There was a problem hiding this comment.
2x typo: It is recommended that each license has a .....
There was a problem hiding this comment.
"license have" is the future perfect tense (I think that is what it is called. "license has" would be past perfect.
src/site/apt/license_def.apt.vm
Outdated
| * Listing components | ||
|
|
||
| All the components (Licenses and Matchers) defined in the system can be displayed using the Documentation | ||
| tool. To run the too execute: |
src/site/apt/license_def.apt.vm
Outdated
| java -jar apache-rat-${project.version}.jar org.apache.rat.Documentation | ||
| +------------------------------------------+ | ||
|
|
||
| The tool take the same arguments as the standard rat CLI, so additional license files can be defined. |
src/site/apt/license_def.apt.vm
Outdated
| tool. To run the too execute: | ||
|
|
||
| +------------------------------------------+ | ||
| java -jar apache-rat-${project.version}.jar org.apache.rat.Documentation |
There was a problem hiding this comment.
Would you mind using the same kind of linking as in
https://creadur.apache.org/rat/apache-rat/index.html
"apache-rat/target/" ?
There was a problem hiding this comment.
We should discuss this. The index.html shows how to call the code if you have built it directly. What we don't do is show how to run the tools if you have downloaded them.
I want to expand tools to include simple scripts to run the basic command line tools given a download of a compressed file, or something similar to that. Make it easy to run for people who are just downloading and running.
src/site/apt/matcher_def.apt.vm
Outdated
|
|
||
| How to define matchers in Apache Rat | ||
|
|
||
| Matchers in Apache Rat are paired with Builders. A Matcher must implement the "IHeaderMatcher" interface and its associated Builder must implement the IHeaderMatcher.Builder interface. |
There was a problem hiding this comment.
Would it make sense to link to the classes within Gitbox? Maybe this helps in case of refactorings as it would yield a 404 .....
src/site/apt/matcher_def.apt.vm
Outdated
| ** The Matcher implementation | ||
|
|
||
| For our example we will implement a Matcher that implements the phrase "Quality, speed and cost, pick any two” by looking for the occurrence of all three words anywhere in the header. | ||
| In most cases is it simplest to extend the AbstractHeaderMatcher class as this class will handle setting of the unique ID for instances that do not otherwise have a unique id. |
There was a problem hiding this comment.
For matters of consistency: is there a difference between ID and id or Id?
I spotted the same in Javadoc of the changes and asked myself if there is a semantic difference between the two or if we should just decide to use one notation in all places.
Personally I'm fine with "Id" in the code and "id" in written text.
There was a problem hiding this comment.
No semantic difference. I have tried to clean the documentation to make it all "id"
src/site/apt/matcher_def.apt.vm
Outdated
| In most cases is it simplest to extend the AbstractHeaderMatcher class as this class will handle setting of the unique ID for instances that do not otherwise have a unique id. | ||
|
|
||
| +------------------------------------------+ | ||
| public interface IHeaderMatcher extends Component { |
There was a problem hiding this comment.
Not sure if there is a way to reference parts of sources, but in case it remains we should stick a big warning in IHeaderMatcher to keep the site up2date.
src/site/apt/matcher_def.apt.vm
Outdated
| +------------------------------------------+ | ||
|
|
||
| +------------------------------------------+ | ||
| public abstract class AbstractHeaderMatcher implements IHeaderMatcher { |
There was a problem hiding this comment.
Maybe it is sufficient to add plain references to both classes as you provide a full example afterwards ?!
src/site/apt/matcher_def.apt.vm
Outdated
| import org.apache.rat.config.parameters.ConfigComponent; | ||
|
|
||
| @ConfigComponent(type = Component.Type.Matcher, name = "QSC", desc = "Reports if the 'Quality, speed and cost, pick any two' rule is violated") | ||
| public class QSCMatcher extends AbstraactHeaderMatcher { |
There was a problem hiding this comment.
Typo: extends AbstractHeaderMatcher
| @Override | ||
| public boolean matches(IHeaders headers) { | ||
| String text = headers.prune() | ||
| return text.contains("quality") && text.contains("speed") && text.contains("cost"); |
There was a problem hiding this comment.
Initially I thought of being able to use one of the Matchers provided within this PR - shouldn't I be able to "concatenate" matchers, such as ..... (pseudocode only):
AndMatcher.match(TextMatcher.contains("quality"), TextMatcher.contains("speed"), TextMatcher.contains("cost"))
But maybe it is just too late now ;) and I misunderstood the example or above matchers in the code.
src/site/apt/matcher_def.apt.vm
Outdated
| In the above example we use the ConfigComponent annotation to identify that this is a Matcher component, that it has the name 'QSC' and a description of what it does. | ||
| If the "name" was not specified it would have been extracted from the class name by removing the "Matcher" from "QSCMatcher". | ||
|
|
||
| The Constructor calls the AbstractHeaderMatcher with a null argument so tha the parent class will assign an id. |
src/site/apt/matcher_def.apt.vm
Outdated
| The builder must implement the IHeaderMatcher.Builder interface. | ||
|
|
||
| +------------------------------------------+ | ||
| @FunctionalInterface |
There was a problem hiding this comment.
same as above I'd prefer to directly link to RAT sources/interfaces/classes via Gitbox instead of copy&pasting them here.
There was a problem hiding this comment.
all links now point to ASF gitbox.
src/site/apt/matcher_def.apt.vm
Outdated
|
|
||
| ** Registering the builder for use in XML configuration | ||
|
|
||
| In order to use the matcher in a Rat configuration it has to be registered with the system. This can be done by creating file with the builder specified and passing it |
There was a problem hiding this comment.
this can be done by creating a file? What kind of file? XML or configuration file?
There was a problem hiding this comment.
Maybe the type just needs to be added for clarification purposes.
There was a problem hiding this comment.
Currently we only have an XML configuration parser but the system is open to new implementations. I have updated the documentation to reflect XML configuration.
src/site/apt/matcher_def.apt.vm
Outdated
| import org.apache.rat.config.parameters.ConfigComponent; | ||
|
|
||
| @ConfigComponent(type = Component.Type.Matcher, name = "TPM", desc = "Checks that the three string are found in the header") | ||
| public class TPMatcher extends AbstraactHeaderMatcher { |
There was a problem hiding this comment.
Typo: AbstractHeaderMatcher
src/site/apt/matcher_def.apt.vm
Outdated
| +------------------------------------------+ | ||
|
|
||
| The ComponentConfigurations with the parameter type indicate that the members specify properties of the component. The Matcher must have a "get" method for each parameter and the builder | ||
| must have a corresponding "set" method. The names of the methods and the attributes in the XML paarser can be changed by adding a 'name' attribute to the ConfigComponent. |
src/site/apt/matcher_def.apt.vm
Outdated
|
|
||
| * Embedded matchers. | ||
|
|
||
| It is possible to create matchers that embed other matchers. The examples in the codebase are the "All", "Any" and "Not" matchers and their associated builders. |
There was a problem hiding this comment.
Could we link to the three matchers in Rat (Gitbox):
- All
- Any
- Not
I'd assume that anyone wanting to add new ones would like to take a closer look at these base matchers.
|
@Claudenw thanks for these big changes with tests and documentation. It's a lot of code that changed, but the examples and tests seem much more simple and straightforward to me. |
Contains all the changes for RAT-355 and RAT-366 as one package.
The two issues were intertwined to the point where multiple files would be updated by both changes and in conflicting ways. This branch contains the changes from both.
The main changes are:
ConfigComponentannotation was added, as was aComponentinterface that is now the root forIHeaderMatcherandILicense, aDescriptionclass that describe the Components, and aDescriptionBuilderthat builds the Descriptions. This results in much simplerIHeaderMatcherandIHeaderMatcher.Buildercode.I am currently working on cleaning up the javadoc but I do not expect any further executable code changes.