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 regression for supporting the default location of the .bootstraprc #147

Merged
merged 18 commits into from
Aug 29, 2016
Merged

Fix regression for supporting the default location of the .bootstraprc #147

merged 18 commits into from
Aug 29, 2016

Conversation

AlexKVal
Copy link
Contributor

@AlexKVal AlexKVal commented Aug 29, 2016

Replaces and continues #142

I've untangled the lot of knots 😄

To facilitate review process I've committed (almost) each step.

I checked both examples with these commands:
general part:

rm -rf node_modules && npm i
cd examples/basic or examples/css-modules
rm -rf node_modules
npm run install-local
npm i

testing:

DEBUG=true npm run bs3
[bootstrap-loader]: bootstrap-loader is using config file at examples/basic/.bootstraprc-3-example 

DEBUG=true npm run bs4
[bootstrap-loader]: bootstrap-loader is using config file at examples/basic/.bootstraprc-4-example

DEBUG=TRUE npm run bs4:default:dev
[bootstrap-loader]: Using config file examples/basic/.bootstraprc 

DEBUG=true npm run bs:no-config
[bootstrap-loader]:  Using config file examples/basic/node_modules/bootstrap-loader/.bootstraprc-3-default

This change is Reviewable

@justin808
Copy link
Member

Reviewed 7 of 11 files at r1.
Review status: 7 of 11 files reviewed at latest revision, 1 unresolved discussion.


src/bootstrap.config.js, line 26 [r1] (raw file):

  if (!config) {
    throw new Error(`I cannot parse the config file at ${configFilePath}'`);

Do we need a try/catch block? I guess the strategy is just to let the error go to the top level.

this is the right error:
Config file cannot be parsed: ${configFilePath}'


Comments from Reviewable

@justin808
Copy link
Member

GREAT JOB @AlexKVal!

:lgtm_strong:

Just the one change to the message....


Reviewed 4 of 11 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@justin808 justin808 merged commit 75d6864 into shakacode:v1 Aug 29, 2016
justin808 pushed a commit that referenced this pull request Aug 30, 2016
#147)

* Fix regression for supporting the default location of the .bootstraprc
* Log the path of config file used
* Global => local variables
* We don't need userConfigFileExists() function
* Simplify a bit setConfigVariables()
* Rename configFilePath => customConfigFilePath
* Rename defaultConfigPath => configFilePath
* Rename configFile => configFilePath
* Rename rawConfig => userConfig
* Begin splitting of setConfigVariables(). Move out readUserConfig() part
* Transform setConfigVariables() => readDefaultConfig()
* Refactor readDefaultConfig()
* DRY a bit readDefaultConfig() and readUserConfig()
* Refactor parseConfigFile()
* Move defaultUserConfigPath. It belongs to readDefaultConfig()
* Examples work only with bootstrap@4.0.0-alpha.2 version
   bootstrap@4.0.0-alpha.3 version throws error
* Fix examples' readme files and npm-scripts
* Update Changelog
@AlexKVal AlexKVal deleted the fix-regression branch August 30, 2016 15:19
justin808 pushed a commit that referenced this pull request Aug 31, 2016
#147)

* Fix regression for supporting the default location of the .bootstraprc
* Log the path of config file used
* Global => local variables
* We don't need userConfigFileExists() function
* Simplify a bit setConfigVariables()
* Rename configFilePath => customConfigFilePath
* Rename defaultConfigPath => configFilePath
* Rename configFile => configFilePath
* Rename rawConfig => userConfig
* Begin splitting of setConfigVariables(). Move out readUserConfig() part
* Transform setConfigVariables() => readDefaultConfig()
* Refactor readDefaultConfig()
* DRY a bit readDefaultConfig() and readUserConfig()
* Refactor parseConfigFile()
* Move defaultUserConfigPath. It belongs to readDefaultConfig()
* Examples work only with bootstrap@4.0.0-alpha.2 version
   bootstrap@4.0.0-alpha.3 version throws error
* Fix examples' readme files and npm-scripts
* Update Changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants