Rename default configuration file #189

Closed
kaelig opened this Issue Aug 27, 2014 · 29 comments

Comments

Projects
None yet
4 participants
@kaelig

kaelig commented Aug 27, 2014

Currently, view.json is parsed by default when running $ sassdoc in the cli.

This is a very generic name that doesn't have its place in the root folder of a project.

I suggest .sassdoc.json instead, which should encourage authors to put the config file at a prime location.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Aug 27, 2014

Member

Obviously a good point, I would also be in favor of a more precise namespace.
Some time ago, It crossed my mind that we could have used something similar to the rc sheme.
.sassdocrc
See https://github.com/dominictarr/rc

Member

pascalduez commented Aug 27, 2014

Obviously a good point, I would also be in favor of a more precise namespace.
Some time ago, It crossed my mind that we could have used something similar to the rc sheme.
.sassdocrc
See https://github.com/dominictarr/rc

@kaelig

This comment has been minimized.

Show comment
Hide comment
@kaelig

kaelig Aug 27, 2014

Even better 👏

kaelig commented Aug 27, 2014

Even better 👏

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 27, 2014

Member

I want this for 2.0. Not before since it will break default API.

Member

HugoGiraudel commented Aug 27, 2014

I want this for 2.0. Not before since it will break default API.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

+1 for a .sassdocsomething, and +1 for the proper rc thing.

Member

valeriangalliat commented Aug 28, 2014

+1 for a .sassdocsomething, and +1 for the proper rc thing.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

SassDoc upgrade script for 2.0

mv view.json .sassdocrc || :

Boom.

Member

valeriangalliat commented Aug 28, 2014

SassDoc upgrade script for 2.0

mv view.json .sassdocrc || :

Boom.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

Oh and @HugoGiraudel, if we don't want to break the API, we can still search in view.json for config, and merge it with rc's output (and gently depreciate the view.json, and definitely removing it for 2.0). But 2.0 is close enough to wait I think.

Member

valeriangalliat commented Aug 28, 2014

Oh and @HugoGiraudel, if we don't want to break the API, we can still search in view.json for config, and merge it with rc's output (and gently depreciate the view.json, and definitely removing it for 2.0). But 2.0 is close enough to wait I think.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member

What I would like to do actually is adding a message right now whenever someone relies on the default naming (view.json) to warn about breaking change in 2.0. Something like:

Starting SassDoc 2.0, the default configuration file won't named view.json anymore.

Regarding this rc thing, it looks cool but it seems like it doesn't support YAML which is coming with 1.6 so I'm not sure.

Member

HugoGiraudel commented Aug 28, 2014

What I would like to do actually is adding a message right now whenever someone relies on the default naming (view.json) to warn about breaking change in 2.0. Something like:

Starting SassDoc 2.0, the default configuration file won't named view.json anymore.

Regarding this rc thing, it looks cool but it seems like it doesn't support YAML which is coming with 1.6 so I'm not sure.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

You're right for the YAML, I'll have a look but I don't think this library will support dynamic parse function according to the extension... maybe we can extend it for this?

I'm not sure with the "merge config with parent" behavior too, if I understood, we can have a .sassdocrc in the package directory, but if there's a parent .sassdocrc, it will be merged too? And the ~/.config/sassdoc/config will too? I prefer to stop at the first .sassdocrc, and use ~/.config/sassdoc/config only if nothing is found.

The thing is .sassdocrc won't work, we need .sassdoc.json or .sassdoc.yaml, and ~/.config/sassdoc/config.yaml or ~/.config/sassdoc/config.json.

What about switching everything to YAML only? SassDoc isn't opinionated after all? But that won't work with the rc library, and I don't like the INI format either.

Member

valeriangalliat commented Aug 28, 2014

You're right for the YAML, I'll have a look but I don't think this library will support dynamic parse function according to the extension... maybe we can extend it for this?

I'm not sure with the "merge config with parent" behavior too, if I understood, we can have a .sassdocrc in the package directory, but if there's a parent .sassdocrc, it will be merged too? And the ~/.config/sassdoc/config will too? I prefer to stop at the first .sassdocrc, and use ~/.config/sassdoc/config only if nothing is found.

The thing is .sassdocrc won't work, we need .sassdoc.json or .sassdoc.yaml, and ~/.config/sassdoc/config.yaml or ~/.config/sassdoc/config.json.

What about switching everything to YAML only? SassDoc isn't opinionated after all? But that won't work with the rc library, and I don't like the INI format either.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member

Why do you want to ditch JSON? Definitely not, we aim for both extensions.

Member

HugoGiraudel commented Aug 28, 2014

Why do you want to ditch JSON? Definitely not, we aim for both extensions.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

Well, I clearly prefer YAML over JSON. If we have to choose one for simplicity, I'd definitely go for YAML.

Anyway I think it won't be possible to use the rc library for our needs.

Member

valeriangalliat commented Aug 28, 2014

Well, I clearly prefer YAML over JSON. If we have to choose one for simplicity, I'd definitely go for YAML.

Anyway I think it won't be possible to use the rc library for our needs.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member
  • Let's drop this rc thing.
  • Starting 1.6, we'll support both JSON and YAML for configuration.
  • Starting 1.6, I'd like to show a message when view.json is automatically used to warn against the name change from 2.0.
  • Starting 2.0, we'll use another more specific name for the default configuration file (to be defined).
  • Starting 2.0, we'll obviously get rid of the warning.

Everything okay?

Member

HugoGiraudel commented Aug 28, 2014

  • Let's drop this rc thing.
  • Starting 1.6, we'll support both JSON and YAML for configuration.
  • Starting 1.6, I'd like to show a message when view.json is automatically used to warn against the name change from 2.0.
  • Starting 2.0, we'll use another more specific name for the default configuration file (to be defined).
  • Starting 2.0, we'll obviously get rid of the warning.

Everything okay?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

I'm okay with this.

And I propose .sassdoc.json / .sassdoc.yaml for the config file.

Member

valeriangalliat commented Aug 28, 2014

I'm okay with this.

And I propose .sassdoc.json / .sassdoc.yaml for the config file.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member

Sold.

Related: #194.

Member

HugoGiraudel commented Aug 28, 2014

Sold.

Related: #194.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Aug 28, 2014

Member

The rc lib was just a mere suggestion to start the discussion. There surely are other ones.
But at the end, I don't think we need to have such a range of possible locations for the config file. Especially ~/.config/.... It's more of a per project thing.

Just out of curiosity I'm currently having a look at how they do it in eslint which allow both json and yaml. https://github.com/eslint/eslint/blob/master/lib/config.js

Member

pascalduez commented Aug 28, 2014

The rc lib was just a mere suggestion to start the discussion. There surely are other ones.
But at the end, I don't think we need to have such a range of possible locations for the config file. Especially ~/.config/.... It's more of a per project thing.

Just out of curiosity I'm currently having a look at how they do it in eslint which allow both json and yaml. https://github.com/eslint/eslint/blob/master/lib/config.js

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

WTF! Looks like the YAML parser can load JSON too. Or you can just use braces in YAML to describe objects and JSON is YAML-compatible... awesome!

Member

valeriangalliat commented Aug 28, 2014

WTF! Looks like the YAML parser can load JSON too. Or you can just use braces in YAML to describe objects and JSON is YAML-compatible... awesome!

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

Well let's have a .sassdocrc everywhere and load it using yaml.safeLoad, this will work for JSON too.

Member

valeriangalliat commented Aug 28, 2014

Well let's have a .sassdocrc everywhere and load it using yaml.safeLoad, this will work for JSON too.

@pascalduez

This comment has been minimized.

Show comment
Hide comment
@pascalduez

pascalduez Aug 28, 2014

Member

Yep, that line caught my eyes too...

Member

pascalduez commented Aug 28, 2014

Yep, that line caught my eyes too...

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member

As long as you make sure JSON is fully supported, I see no reason not to do this.

Member

HugoGiraudel commented Aug 28, 2014

As long as you make sure JSON is fully supported, I see no reason not to do this.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member
  • Parse everything with YAML module: 5a93cb3
  • Add a warning about default config change: e124d2f
Member

valeriangalliat commented Aug 28, 2014

  • Parse everything with YAML module: 5a93cb3
  • Add a warning about default config change: e124d2f
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member

So we can close, right?

Member

HugoGiraudel commented Aug 28, 2014

So we can close, right?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Aug 28, 2014

Member

Well the actual rename to .sassdocrc is still to be done for 2.0.

Member

valeriangalliat commented Aug 28, 2014

Well the actual rename to .sassdocrc is still to be done for 2.0.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Aug 28, 2014

Member

My bad.

Member

HugoGiraudel commented Aug 28, 2014

My bad.

@HugoGiraudel HugoGiraudel changed the title from Default configuration file to Rename default configuration file Sep 8, 2014

@HugoGiraudel HugoGiraudel added the Refacto label Oct 7, 2014

@FWeinb FWeinb referenced this issue Oct 11, 2014

Closed

[Meta] Development SassDoc 2.0 #255

20 of 20 tasks complete
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 13, 2014

Member

@valeriangalliat, can you tackle this when you get a chance? It involves three things:

Member

HugoGiraudel commented Oct 13, 2014

@valeriangalliat, can you tackle this when you get a chance? It involves three things:

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Oct 13, 2014

Member

No need to update the variables called view, and ctx.view?

Member

valeriangalliat commented Oct 13, 2014

No need to update the variables called view, and ctx.view?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 13, 2014

Member

Why would we?

Member

HugoGiraudel commented Oct 13, 2014

Why would we?

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Oct 13, 2014

Member

I don't know, maybe consistency but I don't have a better name in mind anyway.

Member

valeriangalliat commented Oct 13, 2014

I don't know, maybe consistency but I don't have a better name in mind anyway.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Oct 13, 2014

Member

view makes sense imo.

Member

HugoGiraudel commented Oct 13, 2014

view makes sense imo.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
@valeriangalliat

valeriangalliat Oct 13, 2014

Member

Let's keep it.

Member

valeriangalliat commented Oct 13, 2014

Let's keep it.

@valeriangalliat

This comment has been minimized.

Show comment
Hide comment
Member

valeriangalliat commented Oct 13, 2014

Done.

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