[RFR] Graceful relative paths in CLI and config file #364

Merged
merged 3 commits into from Feb 10, 2015

Conversation

Projects
None yet
4 participants
@valeriangalliat
Member

valeriangalliat commented Feb 9, 2015

This needs to be merged with --no-ff in both master and develop after review. Then make publish on master.

valeriangalliat added some commits Feb 9, 2015

Don't set default `dest` in CLI
If no `dest` is passed, docopt would still force it to `sassdoc` and the
rest of the code would think `dest` was set explicitely via CLI, thus
overriding any `dest` from potential configuration file.

@valeriangalliat valeriangalliat changed the title from Graceful relative paths in CLI and config file to [RFR] Graceful relative paths in CLI and config file Feb 9, 2015

@@ -10,7 +10,7 @@ Options:
-h, --help Bring help.
--version Show version.
-v, --verbose Enable verbose mode.
- -d, --dest=<dir> Documentation folder [default: sassdoc].

This comment has been minimized.

@HugoGiraudel

HugoGiraudel Feb 9, 2015

Member

Why did you remove the default value here?

@HugoGiraudel

HugoGiraudel Feb 9, 2015

Member

Why did you remove the default value here?

This comment has been minimized.

@valeriangalliat

valeriangalliat Feb 9, 2015

Member

See commit message and Slack discussion; it was overriding any config-defined dest even if not explicitely given via CLI.

@valeriangalliat

valeriangalliat Feb 9, 2015

Member

See commit message and Slack discussion; it was overriding any config-defined dest even if not explicitely given via CLI.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 9, 2015

Coverage Status

Coverage decreased (-0.18%) to 93.78% when pulling 15d607b on hotfix/cli-theme-relative into bb6838f on master.

Coverage Status

Coverage decreased (-0.18%) to 93.78% when pulling 15d607b on hotfix/cli-theme-relative into bb6838f on master.

@valeriangalliat valeriangalliat self-assigned this Feb 10, 2015

src/environment.js
+ }
+
+ this.dest = this.resolve(this.dest, this.destCli);
+ this.destName = path.relative(process.cwd(), this.dest);

This comment has been minimized.

@pascalduez

pascalduez Feb 10, 2015

Member

I find the keys destName and themeName a bit confusing, since they are actually paths. (relative but still).

@pascalduez

pascalduez Feb 10, 2015

Member

I find the keys destName and themeName a bit confusing, since they are actually paths. (relative but still).

This comment has been minimized.

@valeriangalliat

valeriangalliat Feb 10, 2015

Member

You're right, though themeName can also be the package name (not path)… and it's used only for display.

We could call them destPath and themePath but it would not be relevant for theme in the case of a non-path package name.

I'm thinking the most correct way would be to rename them in displayDest and displayTheme.

But themeName was already present in the interface, I don't think it's documented though, so we can consider it private and might rename it without "breaking".

@valeriangalliat

valeriangalliat Feb 10, 2015

Member

You're right, though themeName can also be the package name (not path)… and it's used only for display.

We could call them destPath and themePath but it would not be relevant for theme in the case of a non-path package name.

I'm thinking the most correct way would be to rename them in displayDest and displayTheme.

But themeName was already present in the interface, I don't think it's documented though, so we can consider it private and might rename it without "breaking".

This comment has been minimized.

@pascalduez

pascalduez Feb 10, 2015

Member

Then yeah, either displayDest and displayTheme.
Or maybe a method display() which could be used like env.display('themeName') ?

@pascalduez

pascalduez Feb 10, 2015

Member

Then yeah, either displayDest and displayTheme.
Or maybe a method display() which could be used like env.display('themeName') ?

This comment has been minimized.

@valeriangalliat

valeriangalliat Feb 10, 2015

Member

You mean env.display('theme')? Not possible since the display value for the theme is figured out at the same time as theme, depending if it's a package or a path. Or env.display() would just be a proxy to the env.displayTheme variable.

@valeriangalliat

valeriangalliat Feb 10, 2015

Member

You mean env.display('theme')? Not possible since the display value for the theme is figured out at the same time as theme, depending if it's a package or a path. Or env.display() would just be a proxy to the env.displayTheme variable.

This comment has been minimized.

@pascalduez

pascalduez Feb 10, 2015

Member

Let's keep it the simpler and DRY possible :-)

@pascalduez

pascalduez Feb 10, 2015

Member

Let's keep it the simpler and DRY possible :-)

* @return {String}
*/
- resolve(file) {
- return path.resolve(this.dir, file);
+ resolve(file, cwd = false) {

This comment has been minimized.

@pascalduez

pascalduez Feb 10, 2015

Member

Similarly here, I would rather have a "from CLI" or "CLI enabled" naming, rather than "themeCli" and "destCLI" which may lead to think there are 2 different things.

@pascalduez

pascalduez Feb 10, 2015

Member

Similarly here, I would rather have a "from CLI" or "CLI enabled" naming, rather than "themeCli" and "destCLI" which may lead to think there are 2 different things.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
Member

pascalduez commented Feb 10, 2015

LGTM

Graceful relative paths in CLI and config file
Before this, `dest` was always relative to CWD even when set from config
file, and if a config file was defined, `theme` was always relative to
it even when set via CLI.

Now for all CLI-passed environment properties, a "friend" `*Cli` prperty
is set to `true` (`themeCli`, `destCli`), so the environment loader can
determine whether it's relative to CWD or config file.

Also paths are now stored absolute, but are always displayed relative to
the CWD for coherent CLI output. If you set `theme: ../../theme` in
`foo/config.yml`, the displayed theme in CLI output will be `../theme`.

This fixes #362.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Feb 10, 2015

Coverage Status

Coverage decreased (-0.18%) to 93.78% when pulling c47dea1 on hotfix/cli-theme-relative into bb6838f on master.

Coverage Status

Coverage decreased (-0.18%) to 93.78% when pulling c47dea1 on hotfix/cli-theme-relative into bb6838f on master.

@valeriangalliat valeriangalliat merged commit c47dea1 into master Feb 10, 2015

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.18%) to 93.78%
Details
continuous-integration/travis-ci The Travis CI build passed
Details

@valeriangalliat valeriangalliat deleted the hotfix/cli-theme-relative branch Feb 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment