Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Provide accessor for SearchEntry object #920

Closed
wants to merge 1 commit into from
Closed

Provide accessor for SearchEntry object #920

wants to merge 1 commit into from

Conversation

ecs-hk
Copy link

@ecs-hk ecs-hk commented Jul 26, 2023

Proposing this small change as a more intuitive way to get attribute values under v3.

Related to my question about, and the response to, issue #919. The code change borrows ideas from #841.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. However, it should be added https://github.com/ldapjs/messages and it must include unit tests.

I think the approach is going in the right direction. It's similar to the direction I thought of overnight. I think we could do this is one of two, or both, ways:

  1. When the attributes are added, decorate the this.attributes object with getters, e.g. something like:
for (const attr of attributes) {
  Object.defineProperty(this.attributes, attr.name, {
    get () {
      return attr.value
    }
  })
}
  1. Define a method on SearchEntry.prototype.attributes and accepts an attribute name to get. This would be the scenario you have defined.

@ecs-hk
Copy link
Author

ecs-hk commented Jul 26, 2023

OK, thanks for the reply. I will review your comments and investigate making the change in https://github.com/ldapjs/messages as a method on SearchEntry.prototype.attributes. (That approach is clearer to me personally, but that's of course subjective.) It may take me a bit to understand the testing for this project, and write appropriate unit tests, but I will try.

@ecs-hk ecs-hk closed this Jul 26, 2023
@jsumners
Copy link
Member

It may take me a bit to understand the testing for this project, and write appropriate unit tests, but I will try.

Do your best and we will sort through any changes needed during review.

I suggest looking at https://github.com/ldapjs/messages/blob/041490fb3a33634b8457c06dd75985daea39ff3c/lib/messages/search-result-entry.js#L85-L101

It may be worth creating an attributes prototype object that extends Array, as right now it is implemented with a simple array literal.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants