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

Allow ignorables to be configured in snow.config #130

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Allow ignorables to be configured in snow.config #130

wants to merge 6 commits into from

Conversation

sgrassie
Copy link
Contributor

I wanted more control over ignoring certain files and folders in my blog repo, e.g. the readme.md. I didn't much like the recommended way of specifying a directory to copy stuff into the root from, and that also seemed a little unwieldy to me, as opposed to being able to explicitly specify in a config file "don't delete this file, man".

I have

Removed all hard coded ignore strings from IoExtensions.cs, in favour of allowing a user configurable section of snow.config to be the source of ignores.

  • Removed static hard coded string arrays
  • Added a func factory to Empty() which defaults to null.
  • Extended snow settings with ignorables property.

Still to do

  • Update PR with discussed changes
  • Update sample snow.config in 'snowsite' (not required based on discussion)
  • Update wiki with new documentation

Example snow.config

# somewhere in snow.config
  "ignorables": [
    "cname",
    "compile.snow.bat",
    "snow.config",
    ".nojekyll",
    ".gitignore",
    ".deployment",
    ".git",
    ".svn",
    "svn",
    "snow",
    "readme.md"
  ]

Removed all hard coded ignore strings from IoExtensions.cs, in favour of allowing a user configurable section of snow.config to be the source of ignores.

- Removed static hard coded string arrays
- Added a func factory to Empty() which defaults to null.
- Extended snow settings with ignorables property.
@phillip-haydon
Copy link
Member

Awesome stuff, I like it, can we add's the defaults in if the ignorables are not present?

i.e if you don't add:

"ignorables": [ ]

Then it add's the current list. If it is empty, then it add's an empty list.

If that makes sense.

Just want to prevent people upgrading then going WTF STUFF BROKE!!!

@sgrassie
Copy link
Contributor Author

I would certainly really dislike to break people's stuff, yeah. Let's see I can capture some requirements then:

When building the settings. 
If the config has no ignorables
Then the defaults should be added to the list of ignorables.

When building the settings. 
If the config has an empty list of ignorables 
Then the ignorables list should be empty. 

When building the settings
If the config contains a list of ignorables
They should be used as the list of ignorables.

The PR currently addresses the third one, but it should be fairly straightforward to cover the other two.

@phillip-haydon
Copy link
Member

Sounds good to me.

When/if we make it to V1 or 2 (can't remember where I'm up to) then we will not have a default list as a breaking change.

This would be a nice feature to have now so don't want it to break.

Create the collection of ignores to filter by the factory method. If it
is null, use an empty list.
When the settings file doesn't contain an ignore list, we should default
to this list.
When the ignorables array in the settings is empty, we should handle it
by not specifying any ignores at all.
When ignorables are specified in the config file, we should add those.

Also refactored out the code which creates the ignorables into a
separate method.
@sgrassie
Copy link
Contributor Author

sgrassie commented Nov 5, 2014

Those requirements are now supported, although it might be best to reconsider the second one, as I just spent five minutes wondering why the repo for my blog repository was broken, and it was because I'd deleted the .git folder when I ran VS to test snow with a config file with an empty list of ignorables.

@cyberzed
Copy link
Contributor

Looks way better 👍

@gsferreira
Copy link

👍 Want it!

@cyberzed
Copy link
Contributor

cyberzed commented Mar 2, 2015

My initial thought was keeping the defaults in the class and then allowing an option for the .snowignore file or something like it, the idea of the ignore file was not to pester the config with to much that doesn't concern the actual configuration of the snow transformation.

@sgrassie
Copy link
Contributor Author

sgrassie commented Mar 2, 2015

I've left the sensible defaults in the class, which get picked up if there
are ignores set in the config.

Personally, I like having the ignores in the config, as all my settings are
all in one place then, but I also don't see a problem with also having the
option of something like a .snowignore file.

On Mon, Mar 2, 2015 at 8:22 AM, Stefan Daugaard Poulsen <
[email protected]> wrote:

My initial thought was keeping the defaults in the class and then allowing
an option for the .snowignore file or something like it, the idea of the
ignore file was not to pester the config with to much that doesn't concern
the actual configuration of the snow transformation.


Reply to this email directly or view it on GitHub
#130 (comment).

@cyberzed
Copy link
Contributor

cyberzed commented Mar 2, 2015

👍 doesn't really matter much to me either 😏

@sgrassie
Copy link
Contributor Author

@phillip-haydon I've merged in the upstream changes.

@phillip-haydon
Copy link
Member

@sgrassie awesome, just trying to fix a build bug then I'll merge.

@sgrassie
Copy link
Contributor Author

sgrassie commented Apr 7, 2015

I've just realised that this PR is pretty messy now, I've tried to rebase it into some sort of sensible state, but I'm pretty sure I fucked that up.

I propose that I close this PR and delete the branch, and open a new PR branched off the current master, and remake my changes there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants