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

Fix default source overriding config file #123

Closed
wants to merge 1 commit into from

Conversation

bpolge-kr
Copy link
Contributor

@bpolge-kr bpolge-kr commented Feb 22, 2023

Description (What)

Because the source property was being defaulted to [] on the CLI, this code would never allow the config file to override the default value.

src/get-context/get-config/index.ts:81

function getConfigByName(name: keyof Syncpack.Config.Public, defaultValue?: any): unknown {
    if (typeof (fromCli as any)[name] !== 'undefined')
      return (fromCli as Syncpack.Config.Public)[name];
    if (typeof (fromRcFile as any)[name] !== 'undefined')
      return (fromRcFile as Syncpack.Config.Public)[name];
}

Essentially, fromCli is always set, so the fromRcFile is never even looked at.

I think the best way to fix this is to allow getConfig() from that file to return a default value if nothing is set. Not sure if that's the best place to put this, but it at least puts the default where every part of the code will pull it from

Justification (Why)

It would be nice if the documentation about the configuration file actually worked the way it says it will

How Can This Be Tested?

This test was incorrect.
src/get-context/get-config/get-config.spec.ts:156-161

it('uses config value when set', () => {
    const disk = mockDisk();
    disk.readConfigFileSync.mockReturnValue({ source: ['projects/*'] });
    const config = getConfig(disk, {});
    expect(R.getExn(config).source).toEqual(['projects/*']);
  });

const config = getConfig(disk, {}) was an incorrect state.

  • In a branch that doesn't have these changes, set it to the correct state const config = getConfig(disk, { source: [] })
    ** You'll see that it throws an error
  • Switch to this branch and the error goes away.

@JamieMason
Copy link
Owner

Thanks @bpolge-kr, I'd been having trouble in this area with some other config values and it looks like I've missed this. I'm away until Sunday but will take a look soon after.

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.

None yet

2 participants