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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize release name for umbrella projects #397

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@joaquimadraz

joaquimadraz commented Feb 13, 2018

The problem

I was checking adding Distillery to my umbrella project and the repository has the domain in the name, eg: distillery.io.
From what I could grasp, for umbrella projects Distillery uses the folder name as release name.
When running mix release.init, for the given example distillery.io the configuration is invalid and does not compile:

  release :distillery.io do
    set version: "0.1.0"
    set applications: [
      :runtime_tools,
      distillery: :permanent
  ]

Summary of changes

Sanitize the folder name allowing only letters, numbers and underscores, everything else is replaced by underscore.
I've extracted the sanitization logic to the Utils module.
As for the test, I don't really like the setup but I'll wait for your feedback on that 馃憤

Checklist

  • New functions have typespecs, changed functions were updated
  • Same for documentation, including moduledocs
  • Tests were added or updated to cover changes
  • Commits were squashed into a single coherent commit
"project_name"
"""
@spec to_underscore(String.t) :: String.t
def to_underscore(text) do

This comment has been minimized.

@bitwalker

bitwalker Feb 13, 2018

Owner

Might make more sense to name this sanitize_name, since the name doesn't really reflect what it does.

@bitwalker

This comment has been minimized.

Owner

bitwalker commented Feb 13, 2018

I think a better test here would be to create a project on the fly, build a release in it, and make sure it starts. This doubles as a test for the process of going from new project to release. I don't think it's necessary to check it in (it's a lot of bloat just to check that it reads the config properly, there is probably a much easier method for doing that much).

@bitwalker

This comment has been minimized.

Owner

bitwalker commented Feb 13, 2018

It occurs to me that perhaps a better approach here would be to just quote the atom, rather than sanitize it. That said, we probably do want to perform some degree of sanitization, since the name is used in a variety of contexts, and it is possible something else might get screwed up with really weird names. So I think this is probably a good approach 馃憤

@joaquimadraz

This comment has been minimized.

joaquimadraz commented Feb 13, 2018

Just noticed that distillery.io is actually an invalid name for an Elixir application:

鉂 mix new distillery.io --umbrella
** (Mix) Application name must start with a letter and have only lowercase letters, numbers and underscore, got: "distillery.io". (...)

I found the problem because I changed my project's name on Github and when cloning it the folder was the domain name. It looks like this is an edge case. It's still worth it to sanitize the name though.

So for the test, I'll create a project and rename the folder before running distillery. Sounds good?

@bitwalker

This comment has been minimized.

Owner

bitwalker commented Feb 13, 2018

Yeah I know Mix won't allow it, but like you said, the edge case is always possible, so it's worth it to sanitize. As for the test, sounds good!

@joaquimadraz

This comment has been minimized.

joaquimadraz commented Feb 15, 2018

I've just pushed the changes based on your feedback.
To add distillery as dependency I'm doing some string replacements on mix.exs which feels a bit clunky but I didn't find any other way of doing it.
How do you feel we should go about it?

@joaquimadraz joaquimadraz changed the title from Sanitise release name for umbrella projects to Sanitize release name for umbrella projects Feb 15, 2018

@joaquimadraz

This comment has been minimized.

joaquimadraz commented Feb 23, 2018

Hey @bitwalker, do you have any feedback on this?

@bitwalker

This comment has been minimized.

Owner

bitwalker commented Mar 10, 2018

I've taken a different approach on this. Rather than guessing the app names based on directory, the init task now actually parses/evaluates the AST of the mix.exs file in each app, and gets the application name from the project definition. This way we can ensure we don't have to replicate Mix semantics for names, we just use whatever was defined.

@bitwalker bitwalker closed this Mar 10, 2018

@joaquimadraz

This comment has been minimized.

joaquimadraz commented Mar 11, 2018

Yeah, sounds like a better approach 馃憤

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