Skip to content

Conversation

@fiedlr
Copy link
Contributor

@fiedlr fiedlr commented Jul 24, 2025

For the reviewer

  • the first commit is just upgrading the client with make api-client, it's automatically generated. It could be regenerated in the rare case it would be broken
  • There's no test workspace yet currently that actually returns a non-empty aggregation fact list. This is a todo.
  • I highlighted areas where I wasn't 100% sure and doublechecking might be good

@fiedlr fiedlr changed the title Afi cq 1236 new preagg dataset endpoints feat: new preagg dataset endpoints Jul 24, 2025
@fiedlr fiedlr force-pushed the afi-cq-1236-new-preagg-dataset-endpoints branch 4 times, most recently from 6b2cbfb to b367f4e Compare July 25, 2025 12:17
@fiedlr fiedlr changed the title feat: new preagg dataset endpoints feat: add declarative api for agg fact Jul 25, 2025
@fiedlr fiedlr changed the title feat: add declarative api for agg fact feat: add api to gooddata-sdk for agg fact Jul 25, 2025
@fiedlr fiedlr force-pushed the afi-cq-1236-new-preagg-dataset-endpoints branch 5 times, most recently from 0ea9d04 to 1d24d79 Compare July 25, 2025 14:39
@fiedlr fiedlr force-pushed the afi-cq-1236-new-preagg-dataset-endpoints branch 5 times, most recently from 51ecebe to 45876bd Compare July 28, 2025 13:57
@fiedlr fiedlr force-pushed the afi-cq-1236-new-preagg-dataset-endpoints branch from 45876bd to 07d03a0 Compare July 28, 2025 13:58
@fiedlr fiedlr marked this pull request as ready for review July 28, 2025 14:00
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)
Copy link
Contributor Author

@fiedlr fiedlr Jul 28, 2025

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

Copy link
Contributor

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)
Copy link
Contributor Author

@fiedlr fiedlr Jul 28, 2025

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.

Copy link
Contributor

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doublecheck

Copy link
Contributor

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:
Copy link
Contributor Author

@fiedlr fiedlr Jul 28, 2025

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

Copy link
Contributor

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.

@fiedlr fiedlr merged commit 2423aeb into gooddata:master Jul 29, 2025
9 checks passed
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