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

replace asserts with nicer ValueErrors in gyb #106

Merged
merged 1 commit into from Dec 22, 2015

Conversation

JHoloboski
Copy link

Saw the fixme in this file on the assert, so:

  • Raise ValueErrors where asserts used to be, giving the user some feedback

@gribozavr
Copy link
Collaborator

Thanks! Please clean up the patch so that there are no unrelated whitespace changes.

@JHoloboski
Copy link
Author

@gribozavr Done!

@@ -464,10 +464,14 @@ def tokenGenerator(self, baseTokens):
if (kind == 'gybBlockOpen'):
# Absorb any '}% <optional-comment> \n'
m2 = gybBlockClose.match(self.template, closePos)
assert m2, "Invalid block closure" # FIXME: need proper error handling here.
if m2 is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this equivalent? I thought the equivalent would be if not m2:.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be also great to add a test for this.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that would be better, fixing.


if context.localBindings['__children__'] is not self.children:
raise ValueError(
"Expected bindings to be {}, got {}".format(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error message isn't precise enough. It should say, I think, "The code is not allowed to mutate __children__".

Here's a test:

%{
__children__ = 42
}%

Copy link
Author

Choose a reason for hiding this comment

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

Sure, that makes sense.

@gribozavr
Copy link
Collaborator

@JHoloboski Thank you! Would you mind squashing 5 commits into one and rebasing on top of current master?

@JHoloboski
Copy link
Author

@gribozavr Done :)

gribozavr added a commit that referenced this pull request Dec 22, 2015
replace asserts with nicer ValueErrors in gyb
@gribozavr gribozavr merged commit 1994b83 into apple:master Dec 22, 2015
@dabrahams
Copy link
Collaborator

Did anyone look at the actual results of this? What does the "nicer feedback" look like?

slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
define SWIFT_CC(swift) macro for DispatchStubs
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
define SWIFT_CC(swift) macro for DispatchStubs

Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
maldahleh pushed a commit to maldahleh/swift that referenced this pull request Oct 26, 2020
Disable grouping temporarily because there are no associated group for lit-test-helper
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
DougGregor pushed a commit to DougGregor/swift that referenced this pull request Apr 28, 2024
Rename _MatchingEngine to _RegexParser.
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.

None yet

3 participants