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

Support notes for table group #601

Merged
merged 9 commits into from
Aug 14, 2024
Merged

Support notes for table group #601

merged 9 commits into from
Aug 14, 2024

Conversation

thonx-holistics
Copy link
Contributor

@thonx-holistics thonx-holistics commented Aug 1, 2024

Summary

  • Update @dbml/parse elements to support notes for Table Group
    • Validator for Table Group
    • Validator for Note
    • Interpreter for Table Group
  • Add a filed to Table Group for notes
  • Update TypeScript types related to Table Group notes
  • Update test cases

Issue

(issue link here)

Lasting Changes (Technical)

(please list down: code changes/things that have wide-effect; new libraries/functions added that can be used by others; examples below)

  • (Added class EmailValidator to validate email address' validity)
  • (Added Tenant#is_trial? check)

Checklist

Please check directly on the box once each of these are done

  • Documentation (if necessary)
  • Tests (integration test/unit test)
  • Integration Tests Passed
  • Code Review

- Update setting list validator of TableGroupValidator

- Add setting list interpreter for TableGroupInterpreter

- Update TableGroup interpreter

- Update model_structure

- Update testcases

- Update pegjs
@thonx-holistics thonx-holistics marked this pull request as ready for review August 7, 2024 10:04
@NQPhuc NQPhuc requested a review from Huy-DNA August 7, 2024 10:11
packages/dbml-core/__tests__/parser/parser.spec.js Outdated Show resolved Hide resolved
packages/dbml-core/src/parse/dbml/parser.pegjs Outdated Show resolved Hide resolved
packages/dbml-core/src/parse/dbmlParser.js Outdated Show resolved Hide resolved
Comment on lines 77 to 94
for (const name in settingMap) {
const attrs = settingMap[name];
switch (name) {
case 'headercolor':
errors.push(...attrs.map((attr) => new CompileError(CompileErrorCode.INVALID_TABLE_SETTING, '\'headercolor\' is not supported', attr)))
break;
case 'note':
if (attrs.length > 1) {
errors.push(...attrs.map((attr) => new CompileError(CompileErrorCode.DUPLICATE_TABLE_SETTING, '\'note\' can only appear once', attr)))
}
attrs.forEach((attr) => {
if (!isExpressionAQuotedString(attr.value)) {
errors.push(new CompileError(CompileErrorCode.INVALID_TABLE_SETTING, '\'note\' must be a string literal', attr.value || attr.name!));
}
});
break;
default:
errors.push(...attrs.map((attr) => new CompileError(CompileErrorCode.INVALID_TABLE_SETTING, `Unknown \'${name}\' setting`, attr)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This only need the default and note case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the headercolor case because Table has it, and we may support it for Table Group in the future

Copy link
Contributor

@Huy-DNA Huy-DNA left a comment

Choose a reason for hiding this comment

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

seems fine by me

@thonx-holistics thonx-holistics added the PR: New Feature 🚀 A type of pull request used for changelog categories label Aug 14, 2024
@Huy-DNA Huy-DNA self-requested a review August 14, 2024 07:14
@NQPhuc NQPhuc merged commit 7135e60 into master Aug 14, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 A type of pull request used for changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants