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

Use cached template if template download fails #4173

Merged
merged 8 commits into from
Jul 29, 2018

Conversation

BraulioVM
Copy link
Contributor

@BraulioVM BraulioVM commented Jul 24, 2018

I saw that issue #3850 was newcomer friendly and so I'm trying to fix it.

I intend to add tests once I make sure I'm doing things properly. For the time being, I have manually tested the feature by:

  1. Properly downloading a valid template $ stack new tmp https://raw.githubusercontent.com/stesta/haskell-templates/master/snap-basic-websockets.hsfiles
  2. Removing the tmp directory: $ rm -r tmp.
  3. Disabling network connectivity on my computer.
  4. Trying to use the template again by executing the command in 1 again.

And it works as I intended. I haven't tried yet with a TemplatePath using the RepoPath constructor but that should be quick to try. I haven't seen what would happen if redownload throws a
RedownloadInvalidResponse instead of a RedownloadHttpError either, but the worst thing that I could see happening is a log error badly formatted. I'll take a look at that case too anyway.

I have tried to make sure nobody was catching a HttpException when calling redownload from any part of the codebase. That could be slightly problematic because, as I'm catching that exception myself at redownload, it would never propagate to the former HttpException catchers, which could have been doing something useful with the exception. I've grepped redownload and there is no function calling it that catches the exception, but I can't be sure about functions higher in the call stack. I would say this is not going to be a problem, but wanted to find out what you think about it.

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

@@ -0,0 +1,284 @@
{-# START_FILE {{name}}.cabal #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file needed somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No! I left that by accident because I needed it for the first implementation of the integration test. I'll remove it.

Copy link
Contributor

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Thanks, looks really good, except that question on the simple.hsfiles template

@BraulioVM
Copy link
Contributor Author

I feel like I could do some more testing (trying with a RepoPath for example, which will be easy). I will do that tonight (in like 6-7 hours).

I believe I should add this to the ChangeLog.md. What about the docs?

@mihaimaruseac
Copy link
Contributor

The only relevant place in the docs on a quick scan seems to be https://docs.haskellstack.org/en/stable/GUIDE/#stack-new but even there it would be offtopic to talk about using cached templates.

@BraulioVM
Copy link
Contributor Author

BraulioVM commented Jul 29, 2018

I stumbled upon some issues that made stack build the whole project every time I made a change in the tests, so I needed more time than expected to finish the PR. I'm not sure what it was but I fixed it by removing the ~/.stack directory.

Anyway, tell me if there's anything missing. I will try to be quick.

Copy link
Contributor

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@mihaimaruseac mihaimaruseac merged commit 11dfee3 into commercialhaskell:master Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants