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

Make codespaces detection shorter #1

Merged
merged 1 commit into from
Aug 23, 2020
Merged

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Aug 22, 2020

At least until we have Masterminds/sprig#258.

@felipecrs felipecrs requested a review from twpayne August 22, 2020 19:38
@twpayne
Copy link
Member

twpayne commented Aug 22, 2020

The reason for the original code was that it makes it easier for people to add their own Codespaces specific configuration: they can include it in the existing if/else/end block. Admittedly I didn't include a comment to document this. Thoughts?

@felipecrs
Copy link
Contributor Author

felipecrs commented Aug 22, 2020

I see. I didn't think about that, probably because I can't imagine what else people would do in the chezmoi.toml.tmpl specifically for Codespaces.

@twpayne
Copy link
Member

twpayne commented Aug 22, 2020

The main difference (as far as I can tell) is that the Codespaces dotfile setup is non-interactive so you can't use interactive template functions like promptString. Consequently my personal .chezmoi.toml.tmpl ends up looking like this:

[data]
  name = "Tom Payne"
{{- if env "CODESPACES" }}
  codespaces = true
  email = "twpayne@gmail.com"
{{- else }}
{{- $email := promptString "email" -}}
  codespaces = false
  email = "{{ $email}}"
  sshKey = "{{ $email }} SSH key"
{{- end }}

@felipecrs
Copy link
Contributor Author

felipecrs commented Aug 22, 2020

You're right. I would personally do this differently:

{{- $codespaces:= env "CODESPACES" | not | not -}}

[data]
  name = "Tom Payne"
  codespaces = {{ $codespaces }}
{{- if $codespaces }}
  email = "twpayne@gmail.com"
{{- else }}
{{- $email := promptString "email" -}}
  email = "{{ $email}}"
  sshKey = "{{ $email }} SSH key"
{{- end }}

Here I create the variable $codespaces and you create the variable $email. I know that chezmoi currently can not read the variables defined in [data] in the template time, otherwise, we could just get rid of those definitions and use {{ .codespaces }} instead.

Is this possible? If yes, I can open an issue for such a feature.

@twpayne
Copy link
Member

twpayne commented Aug 23, 2020

You're right. I would personally do this differently:

Thanks! That's a nice improvement and I've updated my template to use it.

I know that chezmoi currently can not read the variables defined in [data] in the template time, otherwise, we could just get rid of those definitions and use {{ .codespaces }} instead.

Is this possible? If yes, I can open an issue for such a feature.

If I understand correctly, this is not possible. The [data] section is in the config file, which does not exist when the config file template is executed.

@felipecrs
Copy link
Contributor Author

I made some changes and included an example mail address. Do you think it's a good idea?

@twpayne
Copy link
Member

twpayne commented Aug 23, 2020

I made some changes and included an example mail address. Do you think it's a good idea?

No :) This is a template repo for people who are just getting started with chezmoi and templates, so we should make things as easy to understand as possible, even if there are some equivalent code golf ways of achieving the same effect.

@felipecrs
Copy link
Contributor Author

I see. What's your opinion about

{{- $codespaces := env "CODESPACES" | not | not -}}

sourceDir = "{{ .chezmoi.sourceDir }}"
[data]
  codespaces = {{ $codespaces }}

Compared to the original one?

@twpayne
Copy link
Member

twpayne commented Aug 23, 2020

I would go with one similar to what you suggested here:

{{- $codespaces:= env "CODESPACES" | not | not -}}

[data]
  name = "Your name"
  codespaces = {{ $codespaces }}
{{- if $codespaces }}{{/* Codespaces dotfiles setup is non-interactive, so set an email address */}}
  email = "your@email.com"
{{- else }}{{/* Interactive setup, so prompt for an email address */}}
  email = "{{ promptString "email" }}"
{{- end }}

Reasons:

  • Demonstrates several features: detecting codespaces, setting variables everywhere, setting variables depending on codespaces.
  • Makes it very clear where users should add their own variables.

@felipecrs felipecrs force-pushed the felipecrs/short-cs branch 2 times, most recently from 1eca957 to 0afc91e Compare August 23, 2020 18:31
@felipecrs felipecrs merged commit da8e278 into master Aug 23, 2020
@felipecrs felipecrs deleted the felipecrs/short-cs branch August 23, 2020 19:10
@twpayne
Copy link
Member

twpayne commented Aug 23, 2020

Thank you :)

@felipecrs
Copy link
Contributor Author

felipecrs commented Aug 23, 2020

Thank you :)

No need to thanks! :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants