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

refactor: Unify pk and content and timestamps into FIELDS #169

Merged
merged 1 commit into from
Mar 31, 2018

Conversation

maiha
Copy link
Contributor

@maiha maiha commented Mar 30, 2018

In master, when we want to iterate all fields, we have to process three kinds of fields pk, content columns and timestamps as following code. I feel this is a boring work and the code looks difficult.

def to_xxx
  # for PK
  {{ PRIMARY[:name] }}

  # for Content
  {% for name, type in FIELDS %}
     ...

  # for timestamps
  {% if SETTINGS[:timestamps] %}

This PR merges them into one FIELDS, and the primary and timestamp now becomes just wrapper macros to the field. Thus, we can simply iterate FIELDS to scan all fields.

def to_xxx
  {% for name, type in FIELDS %}
     ...
related refactor (Granite::ORM::Base)
  • added CONTENT_FIELDS that removes PK from FIELDS
  • added Base.fields to provide cached field names.
  • added Base.content_fields to provide cached content field names.
  • renamed Base#params to Base#content_values for clarity

Best regards,

@Blacksmoke16
Copy link
Contributor

This would be quite helpful for the bulk-import PR :P

@faustinoaq
Copy link
Contributor

Great, @maiha Thank you so much for all your contributions!!! 😄

@Blacksmoke16 Blacksmoke16 mentioned this pull request Mar 30, 2018
7 tasks
Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

💯 🥇 Nice job. So much better!

@drujensen drujensen merged commit 481f7eb into amberframework:master Mar 31, 2018
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