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.

Sign up for GitHub

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

Jump to bottom

Z columns doc to have different template. #257

Merged
merged 6 commits into from
May 17, 2022
Merged

Z columns doc to have different template. #257

merged 6 commits into from
May 17, 2022

Conversation

navinrathore
Copy link
Contributor

Zingg internal columns (Z columns) should have different template.

@navinrathore navinrathore marked this pull request as ready for review May 13, 2022 04:38
sonalgoyal
sonalgoyal requested changes May 13, 2022
View reviewed changes
core/src/main/java/zingg/documenter/ColumnDocumenter.java Outdated
public String getColumnBaseContent (String col) {
String message = "The field '" + col + "' is internally used by Zingg.";

if (col.equals(ColName.CLUSTER_COLUMN)) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to go in the template, not in the java code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to Template in if else blocks.

core/src/main/java/zingg/documenter/ColumnDocumenter.java Outdated
@@ -26,6 +26,7 @@ public class ColumnDocumenter extends DocumenterBase {

private final String CSV_TEMPLATE = "stopWordsCSV.ftlh";
Copy link
Member

Choose a reason for hiding this comment

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

One thing I seem to have missed in my earlier reviews - we should have column templates which include stop word templates. Then it much easier to keep stuff modular and extend etc as needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved content in a separate template and included in the parent template

@navinrathore
Copy link
Contributor Author

#256

@navinrathore navinrathore mentioned this pull request May 13, 2022
Closed
sonalgoyal
sonalgoyal requested changes May 16, 2022
View reviewed changes
Copy link
Member

@sonalgoyal sonalgoyal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

a. z col list should just be columns that star with z_
b. StopWordDocumenter should be introduced and included in the ColumnDocumenter. All code and logic for StopWords should go in there. We can then add more documenters for columns. eg data quality.

@navinrathore
Copy link
Contributor Author

a. z col list should just be columns that star with z_ b. StopWordDocumenter should be introduced and included in the ColumnDocumenter. All code and logic for StopWords should go in there. We can then add more documents for columns. eg data quality.

b. Moved the StopWords stuff in a separate class
a. z col list is indeed formed from 6 z_ columns. Is there any issue in it?

@sonalgoyal
Copy link
Member

It will be neater to have a check in the pattern for z columns instead of keeping a list

@sonalgoyal
Copy link
Member

Please also rebase this pr as there is a merge conflict

@navinrathore
Copy link
Contributor Author

It will be neater to have a check in the pattern for z columns instead of keeping a list

We go for documentation with marked data as input. It includes field definition fields and only few z_columns. DONT_USE fields also excluded.
But, Documenter is generating docs for all fields (excludes DON'T USE) but includes all z_fields. As of now, only ones part of marked data are shown with hyperlink heading.
There was some issue working with sequence of columns of marked data. then we moved to get information from FieldDefinition and z Col list.

@navinrathore
Copy link
Contributor Author

Please also rebase this pr as there is a merge conflict

resolved conflict.

@navinrathore
Copy link
Contributor Author

It will be neater to have a check in the pattern for z columns instead of keeping a list
isZColumns() check comprises pattern matching "z_"

Z_COLUMN List removal will be studied and will be removed in due time.

@sonalgoyal sonalgoyal merged commit 20d22a0 into zinggAI:main May 17, 2022
@navinrathore navinrathore deleted the zDocForZColumns branch June 1, 2022 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonalgoyal sonalgoyal sonalgoyal requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@navinrathore @sonalgoyal