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

feat: add keyed sections and fields #120

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yordis
Copy link

@yordis yordis commented Dec 9, 2023

closes #117

@yordis
Copy link
Author

yordis commented Dec 14, 2023

@volodymyrZotov any thoughts about this feature?

@volodymyrZotov
Copy link
Contributor

@yordis That's an interesting feature! Thank you for the idea and contribution!
I didn't have a chance to think about the best way to implement it. But feel free to prepare this PR for review and we'll polish or adjust the implementation if needed.

@yordis yordis force-pushed the issue-117 branch 5 times, most recently from 812f44e to a484798 Compare December 15, 2023 23:10
Comment on lines +98 to +121
for _, section := range item.Sections {
t.Errorf("Missing Implementation for %s", section.Label)
//keyedSection := dataSourceData.Get(fmt.Sprintf("keyed_sections.%s", section.Label)).(map[string]interface{})
//dataSourceData.GetRawState()
//if keyedSection == nil {
// t.Errorf("Expected keyed section %v to exist", section.Label)
//}
//if keyedSection["id"] != section.ID {
// t.Errorf("Expected keyed section %v to have id %v got %v", section.Label, section.ID, keyedSection["id"])
//}
//
//for _, field := range item.Fields {
// if field.Section != nil && field.Section.ID == section.ID {
// keyedField := dataSourceData.Get(fmt.Sprintf("keyed_sections.%s.keyed_fields.%s", section.Label, field.Label)).(map[string]interface{})
//
// if keyedField == nil {
// t.Errorf("Expected keyed field %v to exist", field.Label)
// }
// if keyedField["id"] != field.ID {
// t.Errorf("Expected keyed field %v to have id %v got %v", field.Label, field.ID, keyedField["id"])
// }
// }
//}
}
Copy link
Author

Choose a reason for hiding this comment

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

@volodymyrZotov I seriously tried to get this unit test to work, but I do not understand enough what is going with these data types.

Could you take it from here at the very least? 🙏🏻

@yordis yordis marked this pull request as ready for review December 30, 2023 03:31
@yordis
Copy link
Author

yordis commented Dec 30, 2023

I do not think it will work 100% since it seems I can have the same fields with the same label. While I also think being realistic, we wouldnt reuse the same label thou. But still, it is a concern that must be documented. Using a warning but still allowing the last one to win I would expect the behavior to be.

Also, the "default section" is called "see more" with no title?!

Really weird

{
  "overview": {
    "title": "TESTING",
    "ainfo": ""
  },
  "details": {
    "sections": [
      {
        "name": "qz7lnpx7knhq4wtmo4rpazggsi",
        "title": "",
        "fields": [
          {
            "t": "PASS1",
            "n": "rgqehtvnmoyb2ibj72iqkgpi7u",
            "k": "concealed",
            "v": "123",
            "inputTraits": {
              "autocorrection": "no",
              "autocapitalization": "none"
            }
          },
          {
            "t": "PASS",
            "n": "n62laja2e4krsynq6yzz7j2qim",
            "k": "concealed",
            "v": "456",
            "inputTraits": {
              "autocorrection": "no",
              "autocapitalization": "none"
            }
          },
          {
            "t": "",
            "n": "pk76fwy77uwf3lzetbpbrg23je",
            "k": "concealed",
            "v": "empty label",
            "inputTraits": {
              "autocorrection": "no",
              "autocapitalization": "none"
            }
          }
        ]
      },
      {
        "name": "7bjiftbm6vwb3bngw6kfz32u6y",
        "title": "SECTION 2",
        "fields": [
          {
            "t": "section2",
            "n": "s2rrhvsu7nu7xxn3yr6f4wnkyy",
            "k": "concealed",
            "v": "passsection2",
            "inputTraits": {
              "autocorrection": "no",
              "autocapitalization": "none"
            }
          }
        ]
      },
      {
        "name": "add more",
        "title": "",
        "fields": [
          {
            "t": "withoutsection?!",
            "n": "dk3gaaz3n666mu32e3n7tb77i4",
            "k": "concealed",
            "v": "withoutsection?!",
            "inputTraits": {
              "autocorrection": "no",
              "autocapitalization": "none"
            }
          },
          {
            "t": "text",
            "n": "hjf2zjmalftd6blij7holjtesu",
            "k": "string",
            "v": "some text",
            "a": {
              "multiline": "yes"
            },
            "inputTraits": {
              "autocorrection": "no",
              "autocapitalization": "Sentences"
            }
          }
        ]
      }
    ]
  },
  "createdAt": "2023-12-30T03:36:02Z",
  "updatedAt": "2023-12-30T03:37:14Z",
  "faveIndex": 0,
  "trashed": "N",
  "templateUuid": "110",
  "uuid": "....."
}

@yordis yordis marked this pull request as draft January 17, 2024 21:05
@dannysauer
Copy link

Any particular reason not to make each name-based section and field a list, since they're not enforced to be unique? Typical usage would just be basically item.sections["some name"][0].fields["some field"][0].value then, unless someone actually wanted all duplicates That's still waaaaay less gross than the current released module behavior.

If using the ID of the section / field instead of the name, then they wouldn't need to be a list because the IDs are always unique. I guess there would need be section_by_name[][] and section_by_id[] attributes, as well as field_by_name[][] and field_by_id[] (with the field lists maybe being under both the section and top-level). Shrug.

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.

Allow Section Fields LookUp by Title
3 participants