-
Notifications
You must be signed in to change notification settings - Fork 54
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
Supported configuration file extension check does not use the per-generator default #104
Comments
Basically the check: configure-pages/src/set-pages-config.js Lines 80 to 90 in b490f53
should return (It would be better to use So maybe just something like: --- a/src/set-pages-config.js
+++ b/src/set-pages-config.js
@@ -76,12 +78,12 @@ function setPagesConfig({ staticSiteGenerator, generatorConfigFile, siteUrl }) {
const settings = getConfigParserSettings({ staticSiteGenerator, generatorConfigFile, siteUrl })
new ConfigParser(settings).injectAll()
} catch (error) {
- const isSupportedFileExtension = SUPPORTED_FILE_EXTENSIONS.some(ext => generatorConfigFile.endsWith(ext))
+ const isSupportedFileExtension = !generatorConfigFile || SUPPORTED_FILE_EXTENSIONS.some(ext => generatorConfigFile.endsWith(ext))
// Logging
if (!isSupportedFileExtension) { |
I believe the main problem here (our failure to log the actual error message) is resolved by: As for failure to correctly detect config files, I'm going to leave that in another issue: |
@JamesMGreene Thanks for keeping this in mind. The issue here is different than the referenced autodetection. To quote the important bit:
To simplify, if you don't pass any config file and the action uses its own implicit boilerplate, the conditional logic testing whether to log "We were unable to determine…" or "Unsupported configuration file extension…" fails because configure-pages/src/set-pages-config.js Line 85 in fc47e3c
generatorConfigFile.endsWith(ext) is empty/falsy, so it "seems" that all errors are then blamed on extension, when that isn't the case if you're not providing a config at all;)
It's just cosmetics though. |
Ah, sorry, thanks for persevering on explaining this one! 😅 I see what you mean now. 👌 |
No worries;) I'd have already demonstrated it better by proposing a fix just by adding the const isSupportedFileExtension = !generatorConfigFile || SUPPORTED_FILE_EXTENSIONS.some… to avoid blaming any errors without config file present on its extension straight away, however TBH I lack the imagination to properly test such change esp. in edge case error handler and not to introduce any potential surface for more issues stemming from it so I leave that to others… I see you've taken more systematic approach to the whole configs magic so I'm just watching from the passenger seat: Good job! 🎩 |
A fix for this is now included in |
The
setPagesConfig
function uses different error messages depending on whether the generator configuration file has a correct extension. However, the configuration file used for this check is based on the contextgenerator_config_file
input only, rather than the default configuration file for the generator used that is computed by thegetConfigParserSettings
function.As a result, if
generator_config_file
is not specified, then errors within thesetPagesConfig
function will always be blamed on an incorrect file extension.The text was updated successfully, but these errors were encountered: