Provide attributes for git config #434

faustinoaq · 2017-12-04T18:25:10Z

Description of the Change

Add README enhancements
Add attributes for git config (based on crystal tools)

Alternate Designs

No

Benefits

Useful data on default README

Possible Drawbacks

No

faustinoaq · 2017-12-04T18:54:29Z

src/amber/cli/templates/app/README.md.ecr

+shards install
+npm install
+```
+
 Configure the `config/environments/development.yml` and set the `database_url` with your credentials to your database.


Is this line still valid?, Should we add amber encrypt reference to this README too? What steps are needed to setup production.yml? What about .encryption_key?

That might live nicely in a Deployment section

@robacarp I added a link to amber encrypt documentation 👍

faustinoaq · 2017-12-04T19:09:02Z

src/amber/cli/templates/app/README.md.ecr

+
+```
+AMBER_ENV=production
+shards build --production --release <%= @name %>


Maybe we can use production automatically if --release flags is used? WDYT?

# config/application.cr {% if flag :release %} settings.env = "production" {% else %} settings.env = ENV["AMBER_ENV"] if ENV["AMBER_ENV"]? {% end %}

I'd rather not, that defeats the purpose of the environment variable.

Sorry, what does --release do in shards build command? I haven't seen it as a valid flag:

❯ shards --version Shards 0.7.1 (2017-10-12) ❯ shards build -h shards [options] <command> Commands: build [targets] [options] check init install list prune update Options: --no-color --version --production -v, --verbose -q, --quiet -h, --help

Builds an executable with --release flag?

I'd rather not, that defeats the purpose of the environment variable.

@robacarp Yeah I know, that's Why I asked,

Sometimes I just think compiling amber projects for production is so repetitive

# Enable production env to read production.enc AMBER_ENV=production # --production does not install DevDependencies shards insttall --production # --release enable crystal build optimizations for production shards build --release <%= @name %>

Maybe we should add something like amber build -e production

We already have amber run -e production but it runs the binary by default, so I think this is more useful during development.

Of course I can do something like AMBER_ENV=production shards build myproject at works (unoptimized). I just thought maybe we can take advantage from shards --production and crystal --release flags.

Builds an executable with --release flag?

@veelenga Yeah, --release builds executable with release flag, seems to be not documented yet

Hm, I believe shards build --release does check dependencies for you, no need to do shards install beforehand.

I'd like to avoid making the way amber uses the underlying crystal commands and libraries too opaque, because it handicaps people when they need to wander outside the bounds of an ordinary Amber application. Perhaps a command like amber build -e production could be more transparent by echoing out what it's doing as it happens?

Hm, I believe shards build --release does check dependencies for you, no need to do shards install beforehand.

Yeah, you can use shards build --production --release

Perhaps a command like amber build -e production could be more transparent by echoing out what it's doing as it happens?

Yeah I like that 👍

...echoing out what it's doing as it happens?

We should do the same with amber run and amber watch using some kind of flag like --verbose

Well, seems that amber build is useless, we already have shards build, currently I'm thinking on remove amber run too

veelenga · 2017-12-04T22:24:20Z

src/amber/cli/templates/app.cr

+
+    def fetch_github_name
+      default = "[your-github-name]"
+      return default unless system(WHICH_GIT_COMMAND)


system(WHICH_GIT_COMMAND) will be called 3 times (getch_author, fetch_email, fetch_github_name). Better to check whether git exists or not earlier.

Maybe we should change it on crystal itself too?
https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/tools/init.cr

Update

Done! 🎉 crystal-lang/crystal#5355

@veelenga Done! Thank you for your suggestion! 👍

veelenga · 2017-12-04T22:24:22Z

src/amber/cli/templates/app.cr

+
+    def fetch_email
+      return "[your-email-here]" unless system(WHICH_GIT_COMMAND)
+      `git config --get user.email`.strip


User may not also have user.email and user.name configured, so fetch_email and fetch_author will return an empty string (instead of "[your-email-here]").

Well, we already used this without major problems for all crystal projects until now, check: https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/tools/init.cr

Right, it doesn't have major problems, but it just doesn't work in all cases properly and is easy to fix.

@veelenga Done! Thank you for your suggestion! 👍

faustinoaq

Pending for more reviews 👍

faustinoaq · 2017-12-04T23:30:46Z

src/amber/cli/templates/app.cr

+
+    def fetch_github_name
+      default = "[your-github-name]"
+      return default unless system(WHICH_GIT_COMMAND)


Maybe we should change it on crystal itself too?
https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/tools/init.cr

Update

Done! 🎉 crystal-lang/crystal#5355

marksiemers · 2017-12-05T17:32:44Z

Can this write the name and email to the generated shards.yml as well?

elorest

Looks cool to me.

faustinoaq added 4 commits December 4, 2017 13:23

Provide attributes for git config

caebcd6

Add git config data and development usage

108b6a4

Fix installation section

6cb1ebd

Fix typo

27fb645

faustinoaq commented Dec 4, 2017

View reviewed changes

faustinoaq requested review from a team, drujensen, elorest, marksiemers and veelenga December 4, 2017 18:54

faustinoaq commented Dec 4, 2017

View reviewed changes

veelenga reviewed Dec 4, 2017

View reviewed changes

faustinoaq commented Dec 4, 2017

View reviewed changes

Remove database condition

24d859e

faustinoaq added 4 commits December 5, 2017 21:01

Add author and email to shard.yml

dc4e019

Fix wrong var name

00a3a72

Avoid empty user.name or user.email

aa05f1f

Use system

c2ba81f

marksiemers approved these changes Dec 6, 2017

View reviewed changes

faustinoaq mentioned this pull request Dec 6, 2017

Avoid empty author and email crystal-lang/crystal#5355

Merged

faustinoaq added 4 commits December 5, 2017 22:06

Add type

b86763c

Mention amber encrypt and add mysql

580d8f3

Remove variable

923215a

Fix default

fe7823a

elorest approved these changes Dec 6, 2017

View reviewed changes

robacarp approved these changes Dec 6, 2017

View reviewed changes

Clarify fetch code

3109155

elorest merged commit 9cdc372 into master Dec 13, 2017

faustinoaq deleted the fa/git-readme branch December 13, 2017 07:22

eliasjpr added this to the 0.5.0 - Features & Bugfixes milestone Dec 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide attributes for git config #434

Provide attributes for git config #434

faustinoaq commented Dec 4, 2017 •

edited

Loading

faustinoaq Dec 4, 2017

robacarp Dec 5, 2017

faustinoaq Dec 6, 2017

faustinoaq Dec 4, 2017 •

edited

Loading

robacarp Dec 4, 2017

veelenga Dec 4, 2017 •

edited

Loading

faustinoaq Dec 4, 2017

robacarp Dec 5, 2017

faustinoaq Dec 5, 2017

faustinoaq Dec 5, 2017

faustinoaq Dec 6, 2017

veelenga Dec 4, 2017

faustinoaq Dec 4, 2017 •

edited

Loading

faustinoaq Dec 6, 2017

veelenga Dec 4, 2017

faustinoaq Dec 4, 2017

veelenga Dec 5, 2017 •

edited

Loading

faustinoaq Dec 6, 2017

faustinoaq left a comment

faustinoaq Dec 4, 2017 •

edited

Loading

marksiemers commented Dec 5, 2017

elorest left a comment

Provide attributes for git config #434

Provide attributes for git config #434

Conversation

faustinoaq commented Dec 4, 2017 • edited Loading

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Update

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veelenga Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq left a comment

Choose a reason for hiding this comment

faustinoaq Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Update

marksiemers commented Dec 5, 2017

elorest left a comment

Choose a reason for hiding this comment

faustinoaq commented Dec 4, 2017 •

edited

Loading

faustinoaq Dec 4, 2017 •

edited

Loading

veelenga Dec 4, 2017 •

edited

Loading

faustinoaq Dec 4, 2017 •

edited

Loading

veelenga Dec 5, 2017 •

edited

Loading

faustinoaq Dec 4, 2017 •

edited

Loading