Skip to content
This repository has been archived by the owner on Dec 5, 2022. It is now read-only.

Feature/prettyprint #7

Merged
merged 1 commit into from Feb 18, 2020
Merged

Feature/prettyprint #7

merged 1 commit into from Feb 18, 2020

Conversation

FeepingCreature
Copy link
Contributor

@linkrope ping

{
if (got != value)
{
stringCmpError(gotString, valueString, false, file, line);
Copy link

Choose a reason for hiding this comment

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

consider replacing false with No.quoted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

should.terminateChain;

auto got = should.got();
const gotString = got.to!string.prettyprint;
Copy link

Choose a reason for hiding this comment

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

Usually, we do .to!(string[]) in this case. Is .to!string safe?
Or is it possible, that the string representation of one array element contains an unmatched parenthesis that results in an unwanted parse tree of the array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of an unmatched parenthesis, the parse tree will fail to be constructed and we will fall back to ordinary string comparison.

should.addWord!"equal".stringCmp(expected, file, line);
if (got != expected)
{
stringCmpError(got, expected, true, file, line);
Copy link

Choose a reason for hiding this comment

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

Yes.quoted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


/**
* This function takes the input text and returns a pretty-printed, multiline, indented version.
* It assumes that the input text is the output of toString and forms a valid comma separated paren tree.
Copy link

Choose a reason for hiding this comment

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

Add hint like "the output of a structured toString"?

Our toString style is idiosyncratic and this function best suits this style:

  • more machine-readable __repr__ than human-readable __str__
  • one-liner (for better grep results) rather than preformatted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example added


prettyprint("Foo()").should.equal("Foo()");
prettyprint("Foo(Bar(Baz()), Baq())", 16).should.equal(
"Foo(
Copy link

Choose a reason for hiding this comment

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

Consider using correct code indentation and fixing it with outdent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return;
}

result ~= tree.prefix.stripLeft;
Copy link

Choose a reason for hiding this comment

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

Move this up before the if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

result ~= ",";
}
result ~= "\n";
(level + 1).iota.each!((_) { result ~= indent; });
Copy link

Choose a reason for hiding this comment

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

Why not repeat? Comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

repeat and join force an extra allocation.

Appender!string result;

// skip prefix so caller can decide whether or not to strip
void walkOneLine(const Tree tree)
Copy link

Choose a reason for hiding this comment

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

Newspaper style: put this below walk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nested functions aren't reorderable. (And can't be, because they close over scopes.)

tree.children.enumerate.each!((index, child) {
if (index > 0)
{
result ~= ",";
Copy link

Choose a reason for hiding this comment

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

Isn't it ", " - with space after comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spacing is as in the original string.

result ~= tree.parenType.get.closing;
}

void walk(size_t level, const Tree tree)
Copy link

Choose a reason for hiding this comment

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

"walk" suggests how the function is implemented - "traverse" would be a similar bad name, that's only appropriate for a template where the what is a template parameter.

A specific function, however, should better be named for the *what and not for the how: something like render

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

@FeepingCreature FeepingCreature force-pushed the feature/prettyprint branch 2 times, most recently from db5eb36 to 84ab585 Compare February 13, 2020 10:11
@FeepingCreature
Copy link
Contributor Author

Moved to an external package.

@FeepingCreature FeepingCreature merged commit 87e7147 into master Feb 18, 2020
@FeepingCreature FeepingCreature deleted the feature/prettyprint branch February 18, 2020 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants