Skip to content

Feature new manifest#55

Merged
kudj merged 29 commits intomainfrom
feature-new-manifest
Jul 17, 2024
Merged

Feature new manifest#55
kudj merged 29 commits intomainfrom
feature-new-manifest

Conversation

@kudj
Copy link
Copy Markdown
Contributor

@kudj kudj commented Jul 4, 2024

No description provided.

@kudj kudj requested a review from SgtMarmite July 4, 2024 09:24
new manifest support
@kudj kudj force-pushed the feature-new-manifest branch from ec51181 to a4be288 Compare July 4, 2024 14:22
Comment thread src/keboola/component/base.py Outdated
@kudj kudj force-pushed the feature-new-manifest branch from c2e1781 to 48795e9 Compare July 8, 2024 11:32
@kudj kudj force-pushed the feature-new-manifest branch from 0c9c953 to 448f434 Compare July 8, 2024 11:34
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
@kudj kudj force-pushed the feature-new-manifest branch from 8328918 to 5d72cc7 Compare July 9, 2024 07:26
@kudj kudj force-pushed the feature-new-manifest branch 3 times, most recently from fad8e9a to 23bf22e Compare July 9, 2024 12:17
@kudj kudj force-pushed the feature-new-manifest branch from e74dbcf to d88c0f9 Compare July 9, 2024 12:22
@kudj kudj force-pushed the feature-new-manifest branch from 52d0686 to d88d972 Compare July 10, 2024 06:32
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py Outdated
Comment thread src/keboola/component/dao.py
@kudj kudj requested a review from davidesner July 10, 2024 10:47
@kudj kudj force-pushed the feature-new-manifest branch from bfffc96 to 1e5d699 Compare July 11, 2024 14:51
Copy link
Copy Markdown
Collaborator

@davidesner davidesner left a comment

Choose a reason for hiding this comment

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

Vypada to dobre az na par veci co je potreba opravit

for metadata in metadata_list:
if not metadata.get('key') and metadata.get('value'):
continue
key = metadata['key']
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@kudj tohle uz jsem rikal v predchozim commentu, ktery je ted nedohledatelny (protoze jsi delal force-push, na tohle bacha pokazdy musis udelat pull nebo radsi rebase kdyz je v destination zmena. Jinak prepises praci nekomu jinymu). Table metadata ma simplified strukturu v novym manifestu jako key:value, takze {key:x, value:x} nebude fungovat.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kudj kudj requested a review from davidesner July 12, 2024 08:18
@davidesner
Copy link
Copy Markdown
Collaborator

davidesner commented Jul 12, 2024

Uz to vypada dobre, jen par veci:

  • columns porad neni to co jsem myslel, viz inline comment
  • Nekde se flag pro novou manifest strukturu jmenuje native_types, nekde new_manifest. Ve skutecnosti myslim, ze new_manifest je to o cem to doopravdy je a casem az zapomenem ze se novej manifest delal kvuli native typum a ty se stanou standardem, bude to davat vetsi smysl. Sjednotil bych to.
  • Jenom pro zamyslenou davam navrh tady mit misto native_types dat legacy_manifest: Optional[bool] = None A kdyz to bude None, tak se to vyplni automaticky podle toho jestli je KBC_DATA_TYPE_SUPPORT nebo ne. Bude totiz voser delat to rucne v kazdy komponente. A i normalnim userum to usnadni praci, protoze na to nebudou muset myslet. Kdyz to nebude None, tak user explicitne rika co to ma byt.
    • obracena logika legacy_manifest proto, ze standard je normalni, ne naopak. Rikame tim, ze to je moznost na enforce legacy manifestu

Copy link
Copy Markdown
Collaborator

@davidesner davidesner left a comment

Choose a reason for hiding this comment

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

for col in val:
if col not in self.columns:
self.schema[col] = ColumnDefinition()
self.schema[col] = ColumnDefinition()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Promin ale tohle porad neni to co jsem myslel 4f3c6dd#diff-6615ea46ac4ee88c1db70b9f0623e257843f25b60b46d65de76a2c8fdbac64bcR1246 tady to potichu zarve a uzivatel si bude myslet ze se neco stalo, ale nic se nestalo. Zachoval bych tu puvodni funkcionalitu a tady prepsal cely schema a vyhodil ten check

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

změněno, takže uživatel si může přepsat celé schema jen nastavením columns a jenom to vypíše warning?

@kudj
Copy link
Copy Markdown
Contributor Author

kudj commented Jul 15, 2024

@davidesner native_types a new_manifest nahrazeny legacy_manifestem který se nastavuje podle KBC_DATA_TYPE_SUPPORT

Copy link
Copy Markdown
Collaborator

@davidesner davidesner left a comment

Choose a reason for hiding this comment

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

lgtm

@kudj kudj merged commit 21432f4 into main Jul 17, 2024
@kudj kudj deleted the feature-new-manifest branch July 17, 2024 07:19
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.

4 participants