[
http://jira.codehaus.org/browse/JBEHAVE-328?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=229989#action_229989
]
Mauro Talevi commented on JBEHAVE-328:
--------------------------------------
Hi Mike, thanks for the contributed patch. It looks very interesting, but I
have few comments:
1. There is no mention of which JFace dependencies are required. Are they
available on Maven Central repo? If so, it would be great if you could amend
the pom.xml to declare it and prove it builds from CLI. If not, that poses a
problem and we could not accept the patch until the dependencies are uploaded
to Central or somewhere else accessible from network (via mvn, ant or gradle).
2. The patch is for JBehave 2.x (svn) and not for JBehave 3.x (git). Given
that it intended for 3.1 it would help if you could provide the patch directly
for the 3.x branch.
3. Given that the contribution focuses on the integration with JFace toolkit,
it would probably make sense to have a separate module, e.g. jbehave-jface.
Makes for much easier dependency management.
4. There are copyright notices to Yell Group in the Java source files. By
submitting the patch, you are implicitly transfering to the JBehave project the
ownership of the copyright, so the copyright notice should be removed (the
@author tags could and should stay). Even better if you explicitly waived any
claim of copyright from Yell or personally from the authors.
> Creation of a new parser to support the tracking of line numbers in error
> reporting.
> ------------------------------------------------------------------------------------
>
> Key: JBEHAVE-328
> URL: http://jira.codehaus.org/browse/JBEHAVE-328
> Project: JBehave
> Issue Type: Improvement
> Components: Core
> Affects Versions: 3.1
> Reporter: Mike Neeve
> Fix For: 3.1
>
> Attachments: jbehave-core.patch
>
>
> Attached is a patch for JBehave.
> The main change is the addition of a new parser. Its based on Eclipse JFace,
> which means we can track line numbers in error reporting. It can also be
> re-used for syntax highlighting in an Eclipse editor. JFace requires some
> Eclipse libraries, so these are new dependencies which still work even though
> we're not running in an OSGi container. The parser is also very fast compared
> to the current regex-based parser - the longest running test was 25s for the
> previous parser, and now its just 0.5s.
> In ScenarioParserBehaviour we've written some tests for the default
> configuration of JUnitScenario. They fail with the current parser but pass
> with the new one which is able to throw a new ParseException for each type of
> problem.
> We've added one test to CandidateStepBehaviour.java that currently fails.
> Doug tried fixing it by changing the ExamplesTable to be backed by a
> LinkedHashMap which means that the arguments are automatically in the correct
> order. But this did not work for other examples and couldn't figure out how
> to make it work, so the test is currently broken. We'd need help to fix this.
> Further proposed changes would improve the detection of problems such as:
> - not having enough columns in the examples section to cover all the required
> variables
> - having more columns than required in the examples section
> - checking the order of statements e.g. ensure a When comes after a Given
> - allowing boolean's for parameters
> The first two are quite difficult as it involves a good knowledge of the
> CandidateStep class.
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
http://jira.codehaus.org/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email
