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

Add 'status' command [GH#516] #527

Merged
merged 0 commits into from Jan 31, 2017

Conversation

Projects
None yet
3 participants
@perlancar
Contributor

perlancar commented Jan 21, 2017

No description provided.

@perlancar

This comment has been minimized.

Show comment
Hide comment
@perlancar

perlancar Jan 21, 2017

Contributor

This is part of PRC assignment for January 2017.

Contributor

perlancar commented Jan 21, 2017

This is part of PRC assignment for January 2017.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 21, 2017

Coverage Status

Coverage decreased (-0.7%) to 92.494% when pulling d160c3e on perlancar:add-status-command into e3eb705 on preaction:master.

Coverage Status

Coverage decreased (-0.7%) to 92.494% when pulling d160c3e on perlancar:add-status-command into e3eb705 on preaction:master.

@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Jan 26, 2017

Owner

Thanks for implementing this feature! There's one small change to make before I can merge this: The status.yml file should be in the .statocles directory. So Path::Tiny->new( '.statocles', 'status.yml' ) should work fine (see the default value for build_store: https://github.com/preaction/Statocles/blob/master/lib/Statocles/Site.pm#L310).

Because of where the file is being written (in .statocles/status.yml), it will be difficult to actually test any of this, otherwise I'd also ask that you write some automated tests. Right now, the Site object only knows about build_store (which defaults to .statocles/build). Since we can't assume any value for build_store, to fix this problem we will have to create a different attribute. This is way outside the scope of your work, so I'm not going to block this PR on that. I will open a new ticket for the new feature I just described.

So, once the file path is changed to include the .statocles directory, I can merge this. If you'd like me to handle that part, just let me know.

Thanks again for your help!

Owner

preaction commented Jan 26, 2017

Thanks for implementing this feature! There's one small change to make before I can merge this: The status.yml file should be in the .statocles directory. So Path::Tiny->new( '.statocles', 'status.yml' ) should work fine (see the default value for build_store: https://github.com/preaction/Statocles/blob/master/lib/Statocles/Site.pm#L310).

Because of where the file is being written (in .statocles/status.yml), it will be difficult to actually test any of this, otherwise I'd also ask that you write some automated tests. Right now, the Site object only knows about build_store (which defaults to .statocles/build). Since we can't assume any value for build_store, to fix this problem we will have to create a different attribute. This is way outside the scope of your work, so I'm not going to block this PR on that. I will open a new ticket for the new feature I just described.

So, once the file path is changed to include the .statocles directory, I can merge this. If you'd like me to handle that part, just let me know.

Thanks again for your help!

@perlancar

This comment has been minimized.

Show comment
Hide comment
@perlancar

perlancar Jan 26, 2017

Contributor

Hi Doug,

Thanks for your comment and response. I've committed the change of putting status.yml under .statocles/.

Contributor

perlancar commented Jan 26, 2017

Hi Doug,

Thanks for your comment and response. I've committed the change of putting status.yml under .statocles/.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jan 26, 2017

Coverage Status

Coverage decreased (-0.7%) to 92.494% when pulling 43440fb on perlancar:add-status-command into e3eb705 on preaction:master.

Coverage Status

Coverage decreased (-0.7%) to 92.494% when pulling 43440fb on perlancar:add-status-command into e3eb705 on preaction:master.

@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Jan 26, 2017

Owner

Excellent, thanks! Looks like the Travis failure is both intermittent and mildly related to the issue I mentioned, so I'll merge this as-is and open that new ticket to fix the problem of us not having a generic build directory for us to store things like this, the built site, and caches of other kinds of generated data.

Owner

preaction commented Jan 26, 2017

Excellent, thanks! Looks like the Travis failure is both intermittent and mildly related to the issue I mentioned, so I'll merge this as-is and open that new ticket to fix the problem of us not having a generic build directory for us to store things like this, the built site, and caches of other kinds of generated data.

@preaction preaction merged commit 43440fb into preaction:master Jan 31, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-0.7%) to 92.494%
Details
@preaction

This comment has been minimized.

Show comment
Hide comment
@preaction

preaction Jan 31, 2017

Owner

This is merged and will be available in Statocles 0.083. Thanks!

Owner

preaction commented Jan 31, 2017

This is merged and will be available in Statocles 0.083. Thanks!

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