-
Notifications
You must be signed in to change notification settings - Fork 61
feat: modernization part 2 #1748
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
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
You are about to delete 13 region tags.
This comment is generated by snippet-bot.
|
|
Warning: This pull request is touching the following templated files:
|
…and restore optimization
| import {describe, it} from 'mocha'; | ||
|
|
||
| describe('GcRuleBuilder', () => { | ||
| it('has working TypeScript types', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new changes, are there tests anywhere that ensure the full sample from the doc https://docs.google.com/document/d/14aOXOpEbwpYJe7-p6E-PUgoy0vn4AKI7zP0Man0prLo/edit?tab=t.0#heading=h.d7jxtor70ekh runs without errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure there's a very similar one in the table admin tests, but maybe we could update that one to do what's in the doc, since it's more comprehensive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go ahead and plan to do this in a different PR, maybe we can also add the family.js stuff suggested above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nits and checks.
danieljbruce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few more responses.
This is a follow-up for these two PRs:
#1739
#1740
This takes care of some missing pieces that were discovered by further discussion:
There is still a CI issue that will need to be corrected before this can be merged. I don't think it's the result of this change, but I also don't want to chance it.