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

feat:refer viewpoints from tables #530

Closed
wants to merge 1 commit into from

Conversation

majimaccho
Copy link
Contributor

@majimaccho majimaccho commented Oct 22, 2023

Description

Impleemented #526

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

updated golden test files

@@ -15,7 +15,7 @@ import (
// Output is interface for output
type Output interface {
OutputSchema(wr io.Writer, s *schema.Schema) error
OutputTable(wr io.Writer, s *schema.Table) error
OutputTable(wr io.Writer, s *schema.Table, viewpoints schema.Viewpoints) error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@k1LoW
To refer viewpoints from tables, I need to chenge the Output interface and that affects other unrelated files like dot.go.
If you have better idea to implement this feature, kindly let me know.
I appletiate it.

Copy link
Owner

Choose a reason for hiding this comment

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

It may be better to add a Viewpoints field to the Table.

Copy link
Owner

Choose a reason for hiding this comment

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

@macoto1995 You may have it implemented as is. I will correct it on my end!

Copy link
Contributor Author

@majimaccho majimaccho Oct 24, 2023

Choose a reason for hiding this comment

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

@k1LoW
Oh..Sorry I missed your new comment for a few days.

It may be better to add a Viewpoints field to the Table.

I have some concern about this solution.
Since Viewpoint has Tables as children, it would be infinite loop.
Plus, to render link to viewpoints from the markdown, the OutputTable function has to know the index of the viewpoints, which is not included in Viewpoint type.

So I suppose that it's better to have TableViewpoint type like below.

struct TableViewpoint {
    Index Int
    Name String
    Desc String
}

Kindly, let me know what you think.
I'd work on this way for now.
Thank you.

Copy link
Owner

Choose a reason for hiding this comment

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

Since Viewpoint has Tables as children, it would be infinite loop.

Yes. In fact, recursive relationships are everywhere (e.g. Table.ReferencedTables and Table.Column.ParentRelations ).

Plus, to render link to viewpoints from the markdown, the OutputTable function has to know the index of the viewpoints, which is not included in Viewpoint type.

fmfm, indeed. Good point.

So I suppose that it's better to have TableViewpoint type like below.

LGTM. So far I have no better ideas either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll work on this way!

@majimaccho
Copy link
Contributor Author

moved to #532

@majimaccho majimaccho closed this Oct 26, 2023
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.

2 participants