Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure comment handling code. #80

Merged
merged 1 commit into from Jan 17, 2020
Merged

Restructure comment handling code. #80

merged 1 commit into from Jan 17, 2020

Conversation

@Eyas
Copy link
Collaborator

Eyas commented Jan 17, 2020

There was an impossibility in error handling where the onCloseTag
branch's default (throw Error) statement was impossible. This is because
we would always throw in onOpenTag first. We will never see a valid
closed tag without an opening.

Further, a lone closed tag will fail to parse as a close tag anyway. So
this just never happened.

One might be tempted to just remove the default in the name of coverage.
But the default line actually gives us some value: If we ever add
something in open, we might know if we forgot ot add it in close.

But that seems like it should be enforced on lint-time or compile-time,
not just if we're lucky to exercise a condition. Ideally, we should let
the compiler guarantee that any opening tag has a corresponding closing
tag. This way, we keep the benefits of this line, and even improve them
to become a compile-time property, while changing the closing logic
to reflect the impossibility of seeing an unknown closing tag.

There was an impossibility in error handling where the onCloseTag
branch's default (throw Error) statement was impossible. This is because
we would always throw in onOpenTag first. We will never see a valid
closed tag without an opening.

Further, a lone closed tag will fail to parse as a close tag anyway. So
this just never happened.

One might be tempted to just remove the default in the name of coverage.
But the default line actually gives us some value: If we ever add
something in open, we might know if we forgot ot add it in close.

But that seems like it should be enforced on lint-time or compile-time,
not just if we're lucky to exercise a condition. Ideally, we should let
the compiler guarantee that any opening tag has a corresponding closing
tag. This way, we keep the benefits of this line, and even improve them
to become a compile-time property, while changing the closing logic
to reflect the impossibility of seeing an unknown closing tag.
@Eyas Eyas merged commit 01fbf9c into google:master Jan 17, 2020
3 checks passed
3 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 100.0%
Details
@Eyas Eyas deleted the Eyas:comment-coverage branch Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.