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: allow --config with absolute path [gh-0] #589

Merged
merged 5 commits into from
Mar 16, 2023

Conversation

nahuelon
Copy link
Contributor

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

  • allow and handle absolute path in --config flag
  • add test for --config with absolute path.

New Dependency Submission

@nahuelon nahuelon requested review from umutuzgur and clample March 16, 2023 12:03
expect(configDirectory).toEqual(pathToPosix(path.dirname(path.join(process.cwd(), configFile))))

const {
config,
} = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), ['good-config.js'])
} = await loadChecklyConfig(path.join(__dirname, 'fixtures', 'configs'), [filename])
Copy link
Collaborator

@clample clample Mar 16, 2023

Choose a reason for hiding this comment

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

Why don't we pass the output of splitConfigFilePath to the loadChecklyConfig call? If the goal of the test is to make sure that splitConfigFilePath and loadChecklyConfig work nicely together, then that would make more sense to me.

With the current setup, the calls to splitConfigFilePath and loadChecklyConfig are totally separate, so it's like having two different tests. I don't think that it checks that the two work together.

clample
clample previously approved these changes Mar 16, 2023
Copy link
Collaborator

@clample clample left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I added two suggestions, but I think this is good to go as-is.

clample
clample previously approved these changes Mar 16, 2023
@nahuelon nahuelon requested a review from clample March 16, 2023 13:44
umutuzgur
umutuzgur previously approved these changes Mar 16, 2023
@umutuzgur
Copy link
Member

Should I approve it again?

Copy link
Member

@umutuzgur umutuzgur left a comment

Choose a reason for hiding this comment

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

LGTM

@nahuelon nahuelon merged commit 59acfb2 into main Mar 16, 2023
@nahuelon nahuelon deleted the nahuelon/gh-0/allow-absolute-path-config branch March 16, 2023 15:18
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.

3 participants