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

[bugfix #417] delete old files that have been replaced by a directory #1118

Merged
merged 2 commits into from Jul 1, 2013

Conversation

Projects
None yet
5 participants
@maul-esel
Contributor

maul-esel commented May 17, 2013

This also addresses the bug #417, where creating a dir named the same as a file in a previous build creates an error, as does PR #1086. But this code should be more comprehensible than the old one.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 17, 2013

@mattr-, @parkr: does this code make more sense? (it does to me 😄 ) Now your turn to decide which one to take.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 17, 2013

Both PRs should work. As I didn't know how to test this - you can't really ask cucumber to expect a failure, can you? - I tested it with these commands:

mkdir test-site
cd test-site
touch top
../bin/jekyll build --trace
rm top
mkdir top
touch top/bottom
../bin/jekyll build --trace

(for most parts copied from @sporkexec in #417)

@parkr

This comment has been minimized.

Member

parkr commented May 18, 2013

You can totally test this in cucumber :)

Why do this as a part of #cleanup?

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 18, 2013

You can totally test this in cucumber :)

How?

Why do this as a part of #cleanup?

How else? It's about deleting output of previous builds, which #cleanup does. But it misses stuff it should delete, so this PR fixes #cleanup.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 18, 2013

Plus, the required list of directories is already there, no need to collect that data again.

@parkr

This comment has been minimized.

Member

parkr commented May 18, 2013

Oh oh oh, I totally forgot that it was called before write. My bad. This is a good place to put it in that regard. We might take this opportunity to create a Cleaner class, as Code Climate hates this method in particular for its complexity.

Well, cucumber is just an automated way to create and manipulate jekyll sites, right? Do what you described above!

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 18, 2013

We might take this opportunity to create a Cleaner class, as Code Climate hates this method in particular for its complexity.

I had sorted out the file retrievals in other methods of site, but a new class might be even better (from CodeClimate's point of view). New PR after this is merged or in here?

Will look at cucumber - I might need some new step definitions.

@brainkim

This comment has been minimized.

Contributor

brainkim commented May 18, 2013

Haha. I feel like you guys might be placing too much emphasis on code climate now and I feel sorta responsible cuz I was the one who initially put up the link.

@parkr

This comment has been minimized.

Member

parkr commented May 18, 2013

New step definitions are no problemo at all! If you can reuse or extend any of the existing ones, feel free to do that, too.

We use code climate where I work and it generally gives good advice about complexity. The best indicator they have though, in my opinion, is the Churn vs. Quality graph on the Trends page. The classes and modules in the top right of the graph are the ones we need to break out into smaller chunks and reduce contained complexity.

Simpler code is also good news for everyone: it'll be easier than ever to contribute! :)

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented May 19, 2013

@parkr: Turned out one new step would suffice. Anything else left to do?

Btw, would you prefer pull requests to include an update to the history file, or shall we leave that the one merging the PR (e.g. you, mattr-, mojombo, ...) ?

@parkr

This comment has been minimized.

Member

parkr commented Jun 30, 2013

LGTM @mattr-!

And to answer your earlier question from a month ago (whoops!), we prefer to handle the updating of the History.markdown file :)

mattr- added a commit that referenced this pull request Jul 1, 2013

Merge pull request #1118 from maul-esel/dir-bug-take2
[bugfix #417] delete old files that have been replaced by a directory

@mattr- mattr- merged commit 2484833 into jekyll:master Jul 1, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jul 1, 2013

@maul-esel maul-esel deleted the maul-esel:dir-bug-take2 branch Jul 1, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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