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

Deprecate Script #49

Merged
merged 6 commits into from
Dec 23, 2015
Merged

Deprecate Script #49

merged 6 commits into from
Dec 23, 2015

Conversation

zabawaba99
Copy link
Collaborator

PR deprecates the script tag on the .snag.yml as per conversations on #48 . Also added some tests on the parsing side of things.

- Pulled config parsing code out into its own function.
- Starting warning the user that 'script' is deprecated and they should start using 'build'
Removed '-race' from the snag yaml as it causes tests to take quite a while. Having travis do the heavy lifting instead of doing it locally
@zabawaba99 zabawaba99 added this to the v1.1.1 milestone Dec 23, 2015
// if script has something, tell the user it's deprecated
// and set whatever its contents are to build
if len(c.Script) != 0 {
fmt.Println("The use of 'script' in the yaml file has been deprecated and will be removed in the future.")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding a deprecation to the config so that the builder displays the message on every cleanup? That way we don't have to sleep at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that better. That way they always see it and get reminded that we're going to pull the plug.

I see some sort of deprecation message on the config struct which we build while parsing the config. The builder would then check to see if it is set and then output it. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine by me 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

293b852 - Ideally it'd be nice to have it in color but the coloring stuff is inside the vow package. I'd rather wait until we address #21 before we start exporting stuff inside the vow package

screen shot 2015-12-23 at 12 35 41 pm

f, err := os.Create(".snag.yml")
require.NoError(t, err, "could not create snag.yml")

_, err = f.WriteString(content)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to close the file after writing the string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tonkpils added a commit that referenced this pull request Dec 23, 2015
@Tonkpils Tonkpils merged commit b3671a7 into master Dec 23, 2015
@Tonkpils Tonkpils deleted the deprecate-script branch December 23, 2015 18:23
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.

2 participants