-
Notifications
You must be signed in to change notification settings - Fork 60
feat: add api to gooddata-sdk for agg fact #1087
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: add api to gooddata-sdk for agg fact #1087
Conversation
6b2cbfb to
b367f4e
Compare
0ea9d04 to
1d24d79
Compare
risk: low
51ecebe to
45876bd
Compare
45876bd to
07d03a0
Compare
| if dataset.aggregated_facts: | ||
| for aggregated_fact in dataset.aggregated_facts: | ||
| if aggregated_fact.source_column: | ||
| aggregated_fact.source_column = self._change_case(aggregated_fact.source_column, upper_case) |
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.
Doublecheck, not sure if this should be done also for agg facts. I took it from facts
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.
This is ok.
| for fact in dataset.facts or []: | ||
| self.add_title_description_tags(to_translate, fact.title, fact.description, fact.tags) | ||
| for agg_fact in dataset.aggregated_facts or []: | ||
| self.add_title_description_tags(to_translate, None, agg_fact.description, agg_fact.tags) |
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.
Doublecheck, not sure if we should trasnlate also agg facts.
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.
This is ok.
| for fact in dataset.facts or []: | ||
| self.set_title_description_tags(fact, translated) | ||
| for agg_fact in dataset.aggregated_facts or []: | ||
| self.set_title_description_tags(agg_fact, translated) |
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.
Doublecheck
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.
This is ok.
| Content-Length: | ||
| - '172' | ||
| Content-Security-Policy: |
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.
Doublecheck - not sure why headers changed. But it should be ok as these are just mocks
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.
This IMHO changed because it was run against d-c from repo and not the aio. I think it is okay.
For the reviewer
make api-client, it's automatically generated. It could be regenerated in the rare case it would be broken