Skip to content
This repository was archived by the owner on Feb 13, 2019. It is now read-only.

Conversation

@AlexChesser
Copy link
Contributor

Addresses issues found in #525

@peterblazejewicz
Copy link
Member

@AlexChesser
If that is not a problem, could you rebase interactively this PR and reword the second commit (or just squash into single commit). The second commit message could be misleading.
Thanks!

@AlexChesser
Copy link
Contributor Author

Hi @peterblazejewicz I've just seen your request, and have ALSO been doing a bit more work on top of that. I'll clean up the commit.

@AlexChesser
Copy link
Contributor Author

Hi @peterblazejewicz I've gone ahead and done a bit more work (related to issue #527) and compressed them all into a single commit.

This fix addresses both:

#525 (sqlite3 version)
#527 (inefficient nuget redownload)

and it only has a single commit message.

@peterblazejewicz
Copy link
Member

@AlexChesser

Please move #527 related code to new, separate PR.
The reason behind adding sqlite3 package is clear - at least for me. The second topic is about different subject totally.
Please remove comments from code and put them into commit message. The results are the same, while the code is easier to read.

@spboyer

The fix works by adding non-standard package installer for sqlite3, as apt-get offers only 3.7.13 on Ubuntu 14.04:

apt-cache showpkg sqlite3
Package: sqlite3
Versions: 
3.7.13-1+deb7u2 

while EF requires 3.7.15

We could add this either to all Docker files - or as we are already using EJS if/else tags in Dockerfile - add it only to WebApplication project template - as one that uses EF on xplatform.

We need to add this to Dockerfile as aspnet/home or aspnet/Templates do not use Docker or Docker + EntityFramework xplat (at least yet).

Can you review this PR as well?
Thanks!

@AlexChesser
Copy link
Contributor Author

I'll make time to split the commits tonight after work.

Are you sure that removing comments from the dockerfile serves the same result? I expect that a fresh user "dipping their toe" in the world of aspnet / docker / omnisharp etc.. may be looking at this code for the first time and have zero knowledge of the how and why of all this stuff. End of the day they just want to make it work.

By placing expository comments throughout the beginner's code, we can possibly improve understanding and prevent inefficient choices by developers.

For example the fact that docker caches each line of the dockerfile is critical, but (in my opinion) non-trivial information to come by. Someone who didn't know WHY commands were in the docker file in the order they were in might make the mistake of putting things in an inefficient order.

Which could result in a bad "developer experience" or be misconstrued as the .NET or Docker being slow as opposed to the order of commands being critical.

Totally up to you really - unless I hear back I'll go with removing the comments. Personally I've always wanted the code in framework templates to be more verbose and "tutorial like" in the places where the end-developer is using the code.

I suppose I'd frame it as being "easier to use" as opposed to "easier to read"

@AlexChesser
Copy link
Contributor Author

@peterblazejewicz OK - I've created two atomic commits

#530
#531

I'm sorry about my fubmbling around structuring my commits correctly, I've never contributed to open-source projects before. Love the "structure" ... I could learn a lot professionally by doing it this way.

Anyways - I am going to close this "dirty" one and hopefully can pick up the separate discussions in each of the other threads.

@AlexChesser AlexChesser closed this Dec 8, 2015
@peterblazejewicz
Copy link
Member

@AlexChesser You are welcome!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants