Skip to content

Conversion of the spec to Asciidoc#94

Closed
Sanne wants to merge 28 commits intojakartaee:masterfrom
Sanne:asciidoc
Closed

Conversion of the spec to Asciidoc#94
Sanne wants to merge 28 commits intojakartaee:masterfrom
Sanne:asciidoc

Conversation

@Sanne
Copy link
Copy Markdown
Contributor

@Sanne Sanne commented Feb 11, 2016

https://hibernate.atlassian.net/browse/BVAL-502

I don't think we should merge this in master yet as it doesn't have the tooling to extract a valid XML containing all the TCK tests yet.

However these commits seem "stable enough" to me now to be shared, we could merge this in a feature branch so that others can start collaborate on top of these?

I just realize I forgot to update the README, will add a commit about that soon. Essentially to build it, just invoke "ant".

DavideD and others added 20 commits February 3, 2016 16:07
  This will make sure that when we convert to asciidoc the
  right language is used (right now the default is JAVA)
…ecorations

Matching: \+\+([^\+]+)\+\+
To: +$1+
…manual reverts

The regex: \+(.+?)\+

Exceptions to be careful about (reverting manually):

@pattern(regexp="[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,4}"), //email

<xs:pattern value="[0-9]+(\.[0-9]+)*"/>
@gunnarmorling
Copy link
Copy Markdown
Contributor

Looks great overall; Awesome work!

Some minor comments:

  • The text in the pages of the PDF seems quite wide; Could we increase the padding on the left and right sides a bit?
  • E.g. examples 8 and 99 (in HTML) are hightlighted awkwardly; Not sure we can do much about that, seems like a bug in the highlighter?
  • The numbering scheme for examples has changed; IIRC, @hferentschik was running into the same issue when converting the HV guide; Is there a way to keep the current scheme (which restarts example numbering per chapter)?
  • Two lines ahead of example 76 there is broken mark-up: [classname]`MessageInterpolator,
  • Section 5.1.2: The special character '...' is used for the "..." or var-arg method parameters. Can we change it to three actual "."?

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 11, 2016

Section 5.1.2: The special character '...' is used for the "..." or var-arg method parameters. Can we change it to three actual "."?

Great point, and found several more of those. Added a commit to address this: d9d5c57

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 11, 2016

Two lines ahead of example 76 there is broken mark-up: [classname]`MessageInterpolator,

That's an example of why some things are highly puzzling: the markup seems correct to me, but it's not rendering as expected.

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 11, 2016

Fixed that broken markup; had to add an additional space 👎

Also made sure new chapters would start on a new page for PDF.

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 11, 2016

E.g. examples 8 and 99 (in HTML) are hightlighted awkwardly; Not sure we can do much about that, seems like a bug in the highlighter?

Yes many examples have that same issue. I don't think I can fix that unless we change the Java code of the spec API to workaround it ;-)

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 11, 2016

The text in the pages of the PDF seems quite wide; Could we increase the padding on the left and right sides a bit?

I actually like it, if any it's not wide enough for my personal preferences. I just hate to waste so much paper for people printing it out.
To fix such details requires to actually include a PDF style in the build chain. I'd rather do that as a follow up task as it seems quite distracting from the main goals.

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 11, 2016

The numbering scheme for examples has changed; IIRC, @hferentschik was running into the same issue when converting the HV guide; Is there a way to keep the current scheme (which restarts example numbering per chapter)?

Are you sure that's what we want? I think it's possible but require manually numbering.
So two drawbacks:

  • maintenance burden to keep that rule consistently
  • I prefer the universal sequencing, at least people can refer to "Example X" without being ambiguous. And I think people would be ambiguous, as restarting the numbering per chapter is unexpected.

@gunnarmorling
Copy link
Copy Markdown
Contributor

at least people can refer to "Example X" without being ambiguous.

I meant to prefix it with the chapter number, e.g. example 2.1, 2.2, 2.3, ..., 3.1, ... . That's what currently is done in the 1.1 spec HTML.

@hferentschik
Copy link
Copy Markdown
Contributor

I meant to prefix it with the chapter number, e.g. example 2.1, 2.2, 2.3, ..., 3.1, ...

+1 That is my preference for numbering as well. But as far as I know this is still not supported by Asciidoctor. AFAIK they started working on it though.

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 12, 2016

+1 That is my preference for numbering as well.

Ok now I understand. That would be my preference too. I couldn't find a way to customize it.. someone has suggestions? If only I knew the attribute name for the label I could make it as a preprocessor.

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 12, 2016

Pushed a fix for the code highlighter (simply switched to another one, which happens to work and also happens to look nicer IMO)

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 12, 2016

@emmanuelbernard could you send me a PR to this branch with the new Evaluation License? You mentioned wanting it changed.

<delete dir="${document.basedir}/build" />
</target>

<target name="createTckAuditFile">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please restore this target? It's used for creating the audit file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the tck-audit.xsl itself is gone. We need that back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need them back? They won't work with the other changes. If you need them as development reference, they are in git.. I often worked with that deletion reverted in my work-in-progress branch as they are obviously useful to make progress.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can restore them, but I don't understand why you'd delete them in the first place. It's an essential part of the overall mechanism. At least as a "reminder" they should remain in place, the task is not done without this piece working. Also the delete-and-restore obfuscates the history, making it much harder to understand the actual changes needed the these files.

@gunnarmorling
Copy link
Copy Markdown
Contributor

Could you add "lib" and "target" to .gitignore?

@gunnarmorling
Copy link
Copy Markdown
Contributor

One difference I see in the Docbook XML file is the following on line 350. It used to be:

...can target any of the following ElementTypes:

Whereas now it is

...can target any of the following ElementType s:

(space is added after )

Do you think this can be avoided with reasonable effort?

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 15, 2016

Do you think this can be avoided with reasonable effort?

I tried in many ways but I couldn't find one which wouldn't break in some corner cases. This is an example of the kind of polishing which I'd like to see as follow ups rather than blockers for the PR, as someone else might have a better idea on how to do it.

@Sanne
Copy link
Copy Markdown
Contributor Author

Sanne commented Feb 15, 2016

Could you add "lib" and "target" to .gitignore?

I already had done that: 6e73eaa

@emmanuelbernard
Copy link
Copy Markdown
Contributor

I don't think we need a topic branch on the main repo. Making the audit work will require at least a day of work so we can't do that now.
Can you add my commit for the license that I sent you @Sanne ?
And we will have to continue on that PR later.

@gunnarmorling
Copy link
Copy Markdown
Contributor

This is an example of the kind of polishing which I'd like to see as follow ups rather than blockers for the PR

Ok, that's fine with me. In the worst case we can leave that as is, it doesn't matter that much for the audit file.

I've sent PR Sanne#3 against your fork for restoring and updating the XSL transformation that creates the audit file. It looks good overall, the output it generates only deviates a bit from the previous one:

  • Some things related to formatting/style (that extra whitespace above, different characters used for apostrophes (’ vs. ') which are neglectable.
  • Some changed section numberings; It's not quite clear to me where they come from, but we can handle that separately; It seems the original XSL left out some letters in the assertion numbering in some rare cases (a,b,d vs. a, b, c) whereas now it doesn't, which actually is better.

But re-generating the audit file also reveals some glitches in the AsciiDoc that need fixing:

  • Some missing whitespace related to usage of inline markup; E.g. it's now

a bean mapped to the name formatterexposing the vararg method

Whereas it used to be

a bean mapped to the name formatter exposing the vararg method

Also search for "formatterbean", "Message, groupsand payloadare", "${}within", "ConstraintValidatorsupported", "ConstraintValidatorsupporting".

  • Strange markup and removed "+" character:

[code]$$Interceptor.Priority.PLATFORM_AFTER800$$, in other words priority of4800+.

It should be

Interceptor.Priority.PLATFORM_AFTER+800, in other words priority of 4800.

  • "tck-testable" role missing for this para (see here):

IMPLICIT: if [classname]@ValidateOnExecution is on a type (class or interface)...

  • Sections containing "Group conversions declared in XML and via the @ConvertGroup annotation": The "tck-testable" used to contain the following note, too.

@gunnarmorling
Copy link
Copy Markdown
Contributor

Going to close this one, as I'll send a replacement shortly. Thanks for doing the main work of this, @Sanne!

@gunnarmorling gunnarmorling mentioned this pull request Aug 17, 2016
@Sanne Sanne deleted the asciidoc branch June 8, 2019 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants