-
-
Notifications
You must be signed in to change notification settings - Fork 52
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: resolve the right config file and args from cli #20
Conversation
LGTM 🙏 |
src/config.ts
Outdated
const childrenNames = readdirSync(cwd) | ||
const name = childrenNames.some(name => name.includes('bumpp')) ? 'bumpp' : 'bump' | ||
const { config } = await loadConfig<VersionBumpOptions>({ | ||
name: 'bump', | ||
name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good idea to rely on readdirSync, as the config can come from different sources. Instead we could run c12 twice and merge the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I missed that point.
@@ -18,11 +18,18 @@ export async function loadBumpConfig( | |||
overrides?: Partial<VersionBumpOptions>, | |||
cwd = process.cwd(), | |||
) { | |||
const { config: bumppConfig } = await loadConfig<VersionBumpOptions>({ | |||
name: 'bumpp', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a second thought, I think it's better to only read from bump
to avoid confusion, as we already fix the docs for quite a while now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
Description
This PR contains two fixes.
The first is that the document here says that the config name is
bumpp.config.ts
, but the name is actuallybump.config.ts
. Maybe it's better to support both of the names? This will fix #6.And because of adding the
--no-commit
and--no-tag
options inv9.2.0
, this will set thecommit
andtag
from cli args totrue
instead ofundefined
according to https://github.com/cacjs/cac#negated-options. And this will overrides the config from config file. I made some fixes so it can now read the right args from cli. This will fix #13 and #19.Linked Issues
#6 #13 #19
Additional context