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

src/getProperties.js: add prefix support #137

Merged
merged 1 commit into from
May 29, 2024

Conversation

thypon
Copy link
Member

@thypon thypon commented May 29, 2024

No description provided.

Copy link

openai debug - [puLL-Merge] - brave/pull-merge@137

Description

This pull request introduces a change to the getProperties function by adding a prefix parameter. The function now supports filtering and modifying property names based on a specified prefix. This enhancement aims to provide more flexibility for the properties fetched, likely to accommodate different use cases or to standardize property naming.

Changes

Changes

  • action.cjs

    • Modified the call to getProperties to include a new prefix argument:
      - const properties = await getProperties({ owner: context.repo.owner, repo: context.repo.repo, debug, github })
      + const properties = await getProperties({ owner: context.repo.owner, repo: context.repo.repo, debug, github, prefix: 'pull_merge_' })
  • src/getProperties.js

    • Updated the getProperties function signature to accept a new prefix parameter:
      - export default async function getProperties ({ owner, repo, github, githubToken, debug = false }) {
      + export default async function getProperties ({ owner, repo, github, githubToken, debug = false, prefix = '' }) {
    • Refactored the property processing logic to handle properties with and without the specified prefix:
      - return properties.data.reduce((acc, cur) => {
      -   acc[cur.property_name] = cur.value
      -   return acc
      - }, {})
      + 
      + const output = {}
      +
      + // copy properties not starting with prefix to output
      + properties.data.forEach(c => {
      +   if (!c.property_name.startsWith(prefix)) {
      +     output[c.property_name] = c.value
      +   }
      + })
      +
      + // copy properties starting with prefix to output
      + if (prefix) {
      +   properties.data.forEach(c => {
      +     if (c.property_name.startsWith(prefix)) {
      +       const name = c.property_name.substring(prefix.length)
      +       output[name] = c.value
      +     }
      +   })
      + }
      +
      + return output
      ```</details>
      

Copy link

anthropic debug - [puLL-Merge] - brave/pull-merge@137

Description

This PR modifies the getProperties function to support an optional prefix parameter. When provided, the function will only return properties that start with the specified prefix, and will strip the prefix from the returned property names. This allows for more granular control over which properties are retrieved and how they are named.

Changes

Changes

action.cjs:

  • Pass a prefix parameter with value 'pull_merge_' when calling getProperties

src/getProperties.js:

  • Add prefix parameter with default value of empty string
  • Split property handling into two phases:
    1. Copy properties not starting with prefix to output object as-is
    2. If prefix is specified, copy properties starting with prefix to output object, stripping the prefix from the property name
  • Return the constructed output object instead of using reduce

Copy link

bedrock debug - [puLL-Merge] - brave/pull-merge@137

Description

This PR modifies the getProperties function to support a new optional prefix parameter. When provided, the function will filter the properties returned from the GitHub API to only include those that start with the specified prefix. The prefix is then stripped from the property names before they are added to the output object.

The motivation for this change appears to be to allow more control over which properties are returned from the API, and to provide a way to namespace properties to avoid conflicts.

Changes

Changes

action.cjs:

  • Pass a new prefix option to the getProperties function with the value 'pull_merge_'

src/getProperties.js:

  • Add a new optional parameter prefix with a default value of ''
  • Instead of using reduce to build the output object, use a forEach loop to copy properties to the output object
  • If the property name does not start with prefix, copy it to the output object as-is
  • If prefix is provided, do a second pass over the properties and copy those that start with prefix to the output object, stripping the prefix from the property name

Possible Issues

  • If there are existing properties that start with pull_merge_, they will now be included in the output object with the prefix stripped. This could cause unexpected behavior if the code using the output object is not expecting these properties to be present.
  • The function now loops over the properties twice if a prefix is provided, which could impact performance for a large number of properties. Consider using a single loop and conditionally adding the properties to the output object based on the prefix.

@thypon thypon merged commit 040232b into main May 29, 2024
8 checks passed
@thypon thypon deleted the features/prefix-for-getProperties branch May 29, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant