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

config should return undefined when object has no keys #41

Closed
jetersen opened this issue Jan 8, 2021 · 6 comments
Closed

config should return undefined when object has no keys #41

jetersen opened this issue Jan 8, 2021 · 6 comments
Labels
bug Something isn't working

Comments

@jetersen
Copy link

jetersen commented Jan 8, 2021

Inside a custom default the config object can be {} when _extends is used.

_extends: base:test.yaml

To workaround it, you need to write something like this:

  configs
    .map(config => {
      let labels = config.labels ? config.labels : config;
      if (Object.keys(labels).length === 0) labels = undefined
      return {labels: labels || []};
    })

if you returned undefined after deleting the _extends key that would make it easier to merge objects.

@jetersen jetersen added the bug Something isn't working label Jan 8, 2021
@gr2m
Copy link
Contributor

gr2m commented Jan 8, 2021

I think the config should always be an object, so you don't need to add a check every time you want to read any config property. You would need to change your code above to

  configs
    .map(config => {
      if (!config) return { labels: [] }
      return { labels: config.labels ? config.labels : config };
    })

I don't think that would be much better? Or am I missing something?

@jetersen
Copy link
Author

jetersen commented Jan 8, 2021

You can see my use over here btw: crazy-max/ghaction-github-labeler#125

@jetersen
Copy link
Author

jetersen commented Jan 8, 2021

Perhaps the config entry should not be added if the object has no keys and simply provide an empty array of configs or if extends is used the parent config is the only entry.

@jetersen
Copy link
Author

jetersen commented Jan 8, 2021

I think this:

delete requestedRepoFile.config._extends;
const files = [requestedRepoFile];

Could be written as

  delete requestedRepoFile.config._extends; 
  const files = []; 
  if (Object.keys(requestedRepoFile).length !== 0) {
    files.push(requestedRepoFile)
  }

@gr2m
Copy link
Contributor

gr2m commented Jan 8, 2021

It makes more sense, this is a custom merge strategy using the defaults option. I think your use case is easy enough to workaround, and in theory there might be a use case when an empty configuration might be significant. I think the current behavior is easier to understand, removing a config object from the array just because it doesn't have any configuration in it might lead to to unpredictable, hard to debug bugs. I'd prefer to leave it as is, sorry

@jetersen
Copy link
Author

jetersen commented Jan 8, 2021

No worries.

@gr2m gr2m closed this as completed Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants