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

Cleanup on primary key position #172

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Cleanup on primary key position #172

merged 1 commit into from
Feb 2, 2024

Conversation

dogversioning
Copy link
Contributor

This PR makes the following changes:

  • moves primary key to first position in tables, renames it so that it is always id
  • removes test infrastructure to work around random primary key placement
  • adds a few missing derived tables to core unit testing.

Checklist

  • Consider if documentation (like in docs/) needs to be updated
  • Consider if tests should be added
  • Run pylint if you're making changes beyond adding studies
  • Update template repo if there are changes to study configuration

@@ -4,6 +4,8 @@
{%- endif -%}
{%- endmacro -%}

{#- TODO: move secondary/tertiary resource declaration upstream to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are my attempt to address the dangling comment from the prior PR i pulled the trigger on early

Copy link
Contributor

Choose a reason for hiding this comment

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

You didn’t pull early really, I was just being verbose in the comments 😄

with helper.query_console_output(
verbose, drop_view_table, progress, task
) as v:
with helper.query_console_output(verbose, drop_view_table, progress, task):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dangling debug from the last PR

@@ -4,6 +4,8 @@
{%- endif -%}
{%- endmacro -%}

{#- TODO: move secondary/tertiary resource declaration upstream to
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn’t pull early really, I was just being verbose in the comments 😄

@@ -35,18 +35,6 @@
}


class ResourceTableIdPos(IntEnum):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@dogversioning dogversioning merged commit cf0b614 into main Feb 2, 2024
3 checks passed
@dogversioning dogversioning deleted the mg/table_ordering branch February 2, 2024 17:54
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