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

Warn when unsupported ava.config.json files are encountered #2962

Merged
merged 9 commits into from
Mar 6, 2022

Conversation

razor-x
Copy link
Contributor

@razor-x razor-x commented Feb 6, 2022

@novemberborn
Copy link
Member

Thanks for this @razor-x. ava.config.json files have actually never worked, it's not a v4 thing.

I'm thinking we shouldn't print to the console in load-config itself. Perhaps we could return an array of existing ava.config.json paths that were encountered while loading a configuration file, and then do the logging from lib/cli.js?

@razor-x
Copy link
Contributor Author

razor-x commented Feb 13, 2022

🤦🏻 🤦🏻 That's actually hilarious. I'm pretty sure what happened is there are so many tools that have the pattern of putting configs in package.json or tool.json or tool.js I must have just assume AVA was the same way as some point in the past.

Anyway, I agree about not logging from this file. Instead of changing the return signature though, how about passing a callback onUnsupportedConfigPath as an argument and we can pass the logging behavior in from the caller? This would even let me add a test for the callback.

@novemberborn
Copy link
Member

Returning it makes it easier to print a nice list of encountered files, that's harder to do with a callback.

@razor-x
Copy link
Contributor Author

razor-x commented Feb 14, 2022

I looked at updating the return value, but that seemed kinda stressful as it's a breaking change for that function and would require updating its usage in all tests and other files. I ended up opting for the callback instead since it's a backwards compatible change. Hopefully this is ok with you. It still produces a nice list since it calls the function once with an array.

I ran a quick local test on the cli to check the warning. This is what it looks like.

2022-02-13_21-22

@novemberborn
Copy link
Member

Cheers @razor-x. I haven't had a chance to look at this yet though.

@razor-x
Copy link
Contributor Author

razor-x commented Feb 22, 2022

No worries. I fixed the tests.

@novemberborn
Copy link
Member

@razor-x could you allow me to push to your branch? I've got some commits ready that make this return the unsupported files rather than using the callback but I don't seem to be able to push them to the PR.

@razor-x
Copy link
Contributor Author

razor-x commented Mar 1, 2022

@razor-x could you allow me to push to your branch? I've got some commits ready that make this return the unsupported files rather than using the callback but I don't seem to be able to push them to the PR.

Done. Looks like GH does not support that option for org-owned forks, so I just added you to repo.

@razor-x
Copy link
Contributor Author

razor-x commented Mar 1, 2022

image

image

@novemberborn
Copy link
Member

Strange, it's normally not an issue.

@novemberborn
Copy link
Member

@razor-x what do you think of the changes?

@razor-x
Copy link
Contributor Author

razor-x commented Mar 1, 2022

Strange, it's normally not an issue.

I think it's this: isaacs/github#1681 I got into the habit of putting all my forks on a separate org so they don't clutter up my main account.

@razor-x what do you think of the changes?

All good, thanks.

@novemberborn novemberborn changed the title Add unsupported ava.config.json warning Warn when unsupported ava.config.json files are encountered Mar 6, 2022
@novemberborn novemberborn merged commit 9d35593 into avajs:main Mar 6, 2022
@novemberborn novemberborn deleted the warn-on-json-config branch March 6, 2022 14:11
@novemberborn novemberborn restored the warn-on-json-config branch March 6, 2022 14:11
@novemberborn
Copy link
Member

Lovely, thanks @razor-x!

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.

2 participants