Working insertion of hash values; added a few labels in spec

Working insertion of hash values; added a few labels in spec

Hash values are now computed for each "paragraph" starting with \LMHash
(which includes subsequent grammar, dartCode, itemize blocks, but stops
at \section-like commands).  Now addlatexhash.dart expects three arguments
(first the source latex file, then the destination simplified and
hash-value-annotated latex source file, and finally a file name used to
create the list of hash values emitted).  Adjusted testing accordingly.
Added a test for robustness of the hash value generation: It is checked
that lots of different "unimportant" changes make no difference for the
generated hash values (e.g., we can add/remove comments, change white
space, add \commentary{..} etc. without changing the hash values).

In order to ensure that all "structure" commands in the spec have a label,
I added an \LMLabel{..} a handful of places, following the style which is
used throughout the spec.

In dart.sty, the \renewcommand that made \LMHash{} produce a fixed
hash value has been removed such that the actual hash values are now
inserted into the generated spec PDF/DVI file.  Tests have been adjusted
to handle this difference between the spec with and without hash values
when comparing the two.

R=gbracha@google.com, lrn@google.com, ricow@google.com

Committed: https://code.google.com/p/dart/source/detail?r=41658

Reverted: https://code.google.com/p/dart/source/detail?r=41662

Committed: https://code.google.com/p/dart/source/detail?r=41687

[eernst, 2014-10-24 18:02:18]

eernst@google.com changed reviewers:
+ gbracha@google.com

[eernst, 2014-10-24 18:02:19]

Hash values are now computed by addlatexhash.dart and added to the LaTeX source.
 Testing revised and extended accordingly.

@Rico: Couldn't help reinserting some // ------------ lines, it's just much
easier to navigate the file when it has "sections" like that.  ;-)

[gbracha, 2014-10-24 20:32:20]

lgtm

[ricow1, 2014-10-27 10:08:03]

ricow@google.com changed reviewers:
+ lrn@google.com, ricow@google.com

[ricow1, 2014-10-27 10:08:03]

+lasse for actually looking at the parsing (because he will use 1/5 of the time
parsing the parsing compared to me)

>> @Rico: Couldn't help reinserting some // ------------ lines, it's just much
>> easier to navigate the file when it has "sections" like that.  ;-)
So we do have a construct that allows you to eliminate this need and change it
to doc style comments -> classes. There is nothing preventing you from using a
few well structured classes for this - but I don't really care - this is a one
of script that will not be used elsewhere I guess.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:13: //   dart ../../tools/addlatexhash.dart
dartLangSpec.tex out.tex hash.txt
hash.txt - what is this, windows 3.11? :-)

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:193: // Return 'line' without the "\\cmdName\s*{..}"
command starting at
use doc style comments for method comments (///) see:
https://www.dartlang.org/articles/style-guide/#comments

[Lasse Reichstein Nielsen, 2014-10-28 10:12:13]

drive-by comments

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:176: isArg(argRE, line) => argRE.firstMatch(line) !=
null;
=> line.contains(argRE);

Probably reduces to the same thing, but technically it could be faster.
If argRE is always a regexp, and not just a Pattern, you can use
argRE.hasMatch(line).

I'd rename `argRE` to just `pattern`. Saying "arg" on an argument is redundant,
and designates syntactic meaning, not semantic.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:181: findLineNumbers(lines, test()) {
Type of `test` is incorrect, it's typed to be nullary, but is called with an
argument.
Maybe change `test()`  to `bool test(String line)`.

Consider adding return type. It's good documentation.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:201: const BRACE_END = 125; // char code for '}'
Pedantry: Two spaces before '//' comments, and you could align the comments for
better readability. And '.' at the end.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:215: var afterEscape = false; // actually after '{'
Two space before '//'. I think that is a general style rule in Google.
I don't actually know if the Dart style guide says so, but it's a good habit.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:221: if (afterEscape) afterEscape = false; else
braceLevel++; break;
An if-with-an-else should always be put on multiple lines and fully braced.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:223: if (afterEscape) afterEscape = false; else
braceLevel--; break;
Consider checking if braceLevel goes negative.
Otherwise it will accept "}{" as well-braced.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:225: afterEscape = true; break;
Newline after ';'

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:240: removeCommentary(line) {
You are actively removing commentary code from the document.
Have you considered whether it would be less intrusive to just redefine
\commentary to not print anything. IIRC, just adding something like
\long\def\commentary#1{} after the original definition would turn it all into
nothing. That said, I haven't used LaTeX in years.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:263: final hashMarkArgumentRE = new RegExp(r"{.*}");
Escape '{' characters.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:277: gatheredLine += " " + cleanupLine(line);
This will take time O(lines * chars-in-lines), so it might get pretty slow if
it's a long sequence of lines.
Also, it looks like you are simulating Iterable.join(" "), so if possible, see
if you can use that instead.

Maybe collect the lines in a list, and join them at the end, or just find the
end index and join the lines up to that:

 int index;
 for (index = startIndex; index < nextIndex; index++) {
   if (isHashBlockTerminator(lines[index])) break;
 }
 return lines.getRange(startIndex, index).join(" ");

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:339: // latex source
Pedantry: It's "LaTeX" :)
(Yes, I also insist on ASCII and UTF-8)

[Lasse Reichstein Nielsen, 2014-10-31 13:52:39]

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:277: gatheredLine += " " + cleanupLine(line);
Even more "functional":

 lines.getRange(startIndex, nextIndex)
      .takeWhile((line)=>!isHashBlockTerminator(line))
      .join(" ");

but the for-loop is probably more efficient.

[Lasse Reichstein Nielsen, 2014-11-03 11:34:35]

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:201: const BRACE_END = 125; // char code for '}'
Obviously, to align it, it needs to be two *or more* spaces before the "//".

[eernst, 2014-11-03 14:17:47]

Response to Lasse's review plus response to one comment by Rico.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:13: //   dart ../../tools/addlatexhash.dart
dartLangSpec.tex out.tex hash.txt
On 2014/10/27 10:08:03, ricow1 wrote:
> hash.txt - what is this, windows 3.11? :-)

What's wrong with that?  ;-)  The short file names were chosen in order to keep
the command on a single line, for readability.

It would clearly have been nicer to allow for option control (using things like
--latex-output=out.tex and --hash-listing-output=hash.txt) and/or computing the
file names based on the given arguments and some reasonable defaults rather than
rigidly requiring exactly three arguments for every invocation of
addlatexhash.dart, but it would take some time to clarify how to do this so I
chose to use the not-so-user-friendly fixed command format for now.

Presumably, there will be a script or a Makefile containing the actual
invocation, such that creation of the hash-value annotated spec and the
hash-value listing will be a simple, short command anyway.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:176: isArg(argRE, line) => argRE.firstMatch(line) !=
null;
On 2014/10/28 10:12:12, Lasse Reichstein Nielsen wrote:
> => line.contains(argRE);
> 
> Probably reduces to the same thing, but technically it could be faster.
> If argRE is always a regexp, and not just a Pattern, you can use
> argRE.hasMatch(line).
> 
> I'd rename `argRE` to just `pattern`. Saying "arg" on an argument is
redundant,
> and designates syntactic meaning, not semantic.

Done.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:181: findLineNumbers(lines, test()) {
On 2014/10/28 10:12:13, Lasse Reichstein Nielsen wrote:
> Type of `test` is incorrect, it's typed to be nullary, but is called with an
> argument.
> Maybe change `test()`  to `bool test(String line)`.
> 
> Consider adding return type. It's good documentation.

Interesting!  In fact, having worked with types for years, I chose to use a
dynamically typed style because that was the new thing.  I've been told that
dart2js is explicitly typed, and of course that style should be maintained for
dart2js, but this particular piece of code has no connection to existing
(explicitly typed) code, and hence I could make such a decision.  Rico agreed
that this was OK for a tiny stand-alone thing like this.  But I did not realize
that (obviously, looking at it again) the addition of '()' gives 'test' an
argument list of length zero, rather than an unspecified argument list.  I've
removed the '()' and renamed the argument, in order to use the dynamically typed
style consistently.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:193: // Return 'line' without the "\\cmdName\s*{..}"
command starting at
On 2014/10/27 10:08:03, ricow1 wrote:
> use doc style comments for method comments (///) see:
> https://www.dartlang.org/articles/style-guide/#comments

Done.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:201: const BRACE_END = 125; // char code for '}'
On 2014/10/28 10:12:12, Lasse Reichstein Nielsen wrote:
> Pedantry: Two spaces before '//' comments, and you could align the comments
for
> better readability. And '.' at the end.

Done.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:215: var afterEscape = false; // actually after '{'
On 2014/10/28 10:12:11, Lasse Reichstein Nielsen wrote:
> Two space before '//'. I think that is a general style rule in Google.
> I don't actually know if the Dart style guide says so, but it's a good habit.

Couldn't find it in the style guide, but done anyway.  ;)

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:221: if (afterEscape) afterEscape = false; else
braceLevel++; break;
On 2014/10/28 10:12:13, Lasse Reichstein Nielsen wrote:
> An if-with-an-else should always be put on multiple lines and fully braced.

Done.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:223: if (afterEscape) afterEscape = false; else
braceLevel--; break;
On 2014/10/28 10:12:13, Lasse Reichstein Nielsen wrote:
> Consider checking if braceLevel goes negative.
> Otherwise it will accept "}{" as well-braced.

Actually, the latex command will be used on the source, which means that this
type of error will be detected at that point.  It should be acceptable to
silently-accept-and-preserve this error.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:225: afterEscape = true; break;
On 2014/10/28 10:12:11, Lasse Reichstein Nielsen wrote:
> Newline after ';'

Done.  Presumably this does not apply to 'for'?

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:240: removeCommentary(line) {
On 2014/10/28 10:12:13, Lasse Reichstein Nielsen wrote:
> You are actively removing commentary code from the document.
> Have you considered whether it would be less intrusive to just redefine
> \commentary to not print anything. IIRC, just adding something like
> \long\def\commentary#1{} after the original definition would turn it all into
> nothing. That said, I haven't used LaTeX in years.

That wouldn't work in this case, because the string which is being created here
will be used for the computation of an SHA1 hash value, and it is the intention
that this hash value should stay the same no matter which changes are made
inside \commentary{..} and \rationale{..} commands (it shouldn't even matter
whether those commands exist or not).  At the same time, the removal does not
affect the document, because the string being created here is used _only_ for
the computation of this hash value, it does not go into the LaTeX source output.
 Btw. the accompanying test in standalone/io uses exactly such an added
definition to suppress the added hash values, such that the output before and
after the normalization can be compared.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:263: final hashMarkArgumentRE = new RegExp(r"{.*}");
On 2014/10/28 10:12:13, Lasse Reichstein Nielsen wrote:
> Escape '{' characters.

Done, here and in several other similar locations.

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:277: gatheredLine += " " + cleanupLine(line);
On 2014/10/31 13:52:39, Lasse Reichstein Nielsen wrote:
> Even more "functional":
> 
>  lines.getRange(startIndex, nextIndex)
>       .takeWhile((line)=>!isHashBlockTerminator(line))
>       .join(" ");
> 
> but the for-loop is probably more efficient.

Done, with some adjustments.  Nice!  ;)

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:339: // latex source
On 2014/10/28 10:12:12, Lasse Reichstein Nielsen wrote:
> Pedantry: It's "LaTeX" :)
> (Yes, I also insist on ASCII and UTF-8)

Done.  ;-)

[Lasse Reichstein Nielsen, 2014-11-03 16:32:04]

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/1/tools/addlatexhash.dart#newc...
tools/addlatexhash.dart:225: afterEscape = true; break;
Not to 'for' no. It's a matter of "only one statement per line".

[eernst, 2014-11-04 15:30:20]

Implemented support for sectioning (i.e., the listing file now contains logical
location markers like 'sec:ecmaScope' and 'subsubsec:optionalFormals'). 
Adjusted dart.sty to not add 'sec:' in front of sectioning commands, because
there was a need for something like 'par:' in the case of \paragraph{}.  Added a
Makefile in docs/language in order to ease creation of the PDF/DVI output for
the spec and the spec-with-location-markers, and for cleanup.

[ricow1, 2014-11-11 07:04:09]

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
File tests/standalone/io/addlatexhash_test.dart (right):

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:63: // set up /tmp directory to hold
output
that comment is not true on windows

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:106: } else throw "normalization
failed to produce same result";
use block for else

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:107: } else sameCount++;
use block for else

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:140: final listName = specName +
"-list";
use string interpolation

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:165: tmpStySink.close().then((_) {
you can easily avoid mixing this async call in here if you want, File has a
writeAsStringSync that takes an optional parameter mode which can be set to
append, i.e., you can do:
var extraContent = r"\renewcommand{\LMHash}[1]{\OriginalLMHash{xxxx}}";
new File(styPath).copySync(tmpStyPath).writeAsStringSync(extraContent, mode:
FileMode.APPEND);

[eernst, 2014-11-11 07:37:23]

eernst@google.com changed reviewers:
+ floitsch@google.com

[eernst, 2014-11-11 07:37:24]

(Adding Florian on CC)

[eernst, 2014-11-11 08:00:40]

Adjustments according to review.

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
File tests/standalone/io/addlatexhash_test.dart (right):

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:63: // set up /tmp directory to hold
output
On 2014/11/11 07:04:09, ricow1 wrote:
> that comment is not true on windows

Done.

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:106: } else throw "normalization
failed to produce same result";
On 2014/11/11 07:04:09, ricow1 wrote:
> use block for else

Done.

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:107: } else sameCount++;
On 2014/11/11 07:04:09, ricow1 wrote:
> use block for else

Done.

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:140: final listName = specName +
"-list";
On 2014/11/11 07:04:09, ricow1 wrote:
> use string interpolation

Done.

https://codereview.chromium.org/652993005/diff/40001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:165: tmpStySink.close().then((_) {
On 2014/11/11 07:04:09, ricow1 wrote:
> you can easily avoid mixing this async call in here if you want, File has a
> writeAsStringSync that takes an optional parameter mode which can be set to
> append, i.e., you can do:
> var extraContent = r"\renewcommand{\LMHash}[1]{\OriginalLMHash{xxxx}}";
> new File(styPath).copySync(tmpStyPath).writeAsStringSync(extraContent, mode:
> FileMode.APPEND);

Aha, I can see that writeAsStringSync includes 'close' and all; convenient!  ;-)
 Done.

[ricow1, 2014-11-11 08:01:46]

I don't know if Lasse has more input, but lgtm

[Lasse Reichstein Nielsen, 2014-11-11 08:13:49]

lgtm

https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart#...
tools/addlatexhash.dart:36: final whitespaceRE = new RegExp(r"[ \t]{2,}");
Why is one "whitespace" using \s and the other [ \t]?
At least worth a comment.

[eernst, 2014-11-11 09:04:54]

Fixed bug mentioned in review.

https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart#...
tools/addlatexhash.dart:36: final whitespaceRE = new RegExp(r"[ \t]{2,}");
On 2014/11/11 08:13:48, Lasse Reichstein Nielsen wrote:
> Why is one "whitespace" using \s and the other [ \t]?
> At least worth a comment.

That's indeed wrong --- the intention is to match white space except
end-of-line.  According to
http://regexlib.com/CheatSheet.aspx?AspxAutoDetectCookieSupport=1 this is
specified as [ \f\t\v], but according to
http://ecma-international.org/ecma-262/5.1/#sec-15.10 (referred from
https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:core.RegExp)
we have \s ~ WhiteSpace|LineTerminator, i.e., we want WhiteSpace,

  WhiteSpace :: <TAB>|<VT>|<FF>|<SP>|<NBSP>|<BOM>|<USP>;

which means that we would need to match <NBSP>, <BOM>, <USP> as well.  What is
the recommended approach to these (UTF-8-only) chars in a Dart RegExp context?

Currently I've used [ \f\t\v], and added a short comment.

[Lasse Reichstein Nielsen, 2014-11-11 09:13:23]

https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart
File tools/addlatexhash.dart (right):

https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart#...
tools/addlatexhash.dart:36: final whitespaceRE = new RegExp(r"[ \t]{2,}");
There is no special recommendation in a RegExp context, it only depends on the
use-case - i.e., they work, so if you want/need them, you can add them.

The (not really recommended because it's unreadable) RegExp-only way to match
white-space-but-not-line-terminator without listing them would be '(?:(?=\s).)'.
it exactly matches a single character that is both white-space and
non-line-terminator.

If you have already split into lines at line-terminators, you know that the line
contains no further line terminators, so using \s is safe to use on lines, but
not the entire text. A comment is enough in that case.

[eernst, 2014-11-11 09:46:30]

On 2014/11/11 09:13:23, Lasse Reichstein Nielsen wrote:
> https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart
> File tools/addlatexhash.dart (right):
> 
>
https://codereview.chromium.org/652993005/diff/60001/tools/addlatexhash.dart#...
> tools/addlatexhash.dart:36: final whitespaceRE = new RegExp(r"[ \t]{2,}");
> There is no special recommendation in a RegExp context, it only depends on the
> use-case - i.e., they work, so if you want/need them, you can add them.
> 
> The (not really recommended because it's unreadable) RegExp-only way to match
> white-space-but-not-line-terminator without listing them would be
'(?:(?=\s).)'.
> it exactly matches a single character that is both white-space and
> non-line-terminator.
> 
> If you have already split into lines at line-terminators, you know that the
line
> contains no further line terminators, so using \s is safe to use on lines, but
> not the entire text. A comment is enough in that case.

Cool!  Using the '(?:(?=\s).)' magic, keeping the comment due to unreadability. 
It should work on lines containing end-of-line characters, so a plain \s would
be dangerous.

[eernst, 2014-11-11 09:48:34]

Improved tricky RegExp

[Lasse Reichstein Nielsen, 2014-11-11 09:58:58]

lgtm

[eernst, 2014-11-11 10:08:25]

Committed patchset #6 (id:100001) manually as 41658 (presubmit successful).

[eernst, 2014-11-11 11:53:07]

Had to revert ('no such file'), patch set 7 is after revert.

[eernst, 2014-11-11 15:44:06]

On 2014/11/11 11:53:07, eernst wrote:
> Had to revert ('no such file'), patch set 7 is after revert.

Turned out to be caused by a PATH difference: 'dart' is on the path on my
desktop, but not on buildbot machines, so anything that runs 'dart' as such
fails.  Currently waiting for an update conflict in the spec to be resolved,
because that makes the test in this patch fail.

[eernst, 2014-11-12 08:41:25]

A problem was caused by not having 'dart' on the PATH on buildbots; this was
solved using Platform.executable.

[eernst, 2014-11-12 08:42:03]

Committed patchset #8 (id:140001) manually as 41687 (presubmit successful).