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: display a warning when file exists #81

Merged
merged 1 commit into from Oct 31, 2018
Merged

Conversation

jekyllbot
Copy link
Contributor

@jekyllbot jekyllbot commented Oct 31, 2018

We shouldn't raise an argument error when a file already exist.
This uses logger.abort_with to reflect jekyll new behavior.

@DirtyF DirtyF requested a review from a team October 31, 2018 16:46
Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Fine by me as long as the tests are running properly.

capture_stdout { described_class.process(args) }
}).to raise_error("A draft already exists at _drafts/an-existing-draft.md")
it "displays a warning" do
output = capture_stdout { described_class.process(args) }
Copy link
Member

Choose a reason for hiding this comment

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

Does abort_with not kill the test process?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it does, the tests are passing but are wrong. If I change the string in the test, it still passes. How should I test that we abort?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can simply warn and return instead of a hard abort..

Copy link
Member

Choose a reason for hiding this comment

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

With a simple return file gets overwritten. I thought this was workin' after testing it locally but this isn't, shouldn't have merged 😕

@DirtyF DirtyF changed the title fix: display a warning when file exists WIP fix: display a warning when file exists Oct 31, 2018
@DirtyF DirtyF changed the title WIP fix: display a warning when file exists fix: display a warning when file exists Oct 31, 2018
@DirtyF
Copy link
Member

DirtyF commented Oct 31, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 94a3025 into master Oct 31, 2018
@jekyllbot jekyllbot deleted the pull/color-warning branch October 31, 2018 21:30
jekyllbot added a commit that referenced this pull request Oct 31, 2018
@DirtyF DirtyF restored the pull/color-warning branch October 31, 2018 21:52
DirtyF added a commit that referenced this pull request Oct 31, 2018
DirtyF added a commit that referenced this pull request Oct 31, 2018
@DirtyF DirtyF deleted the pull/color-warning branch November 1, 2018 11:44
@jekyll jekyll locked and limited conversation to collaborators Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants