(PDK-1354) Default template ref for custom templates should always be master #677
Conversation
| @@ -207,11 +207,14 @@ def self.templates(opts) | |||
| end | |||
|
|
|||
| # @returns String | |||
| def self.default_template_ref | |||
| def self.default_template_ref(uri = nil) | |||
There was a problem hiding this comment.
This seems a little complex. Maybe this is better (not sure)?
def self.default_template_ref(uri = nil)
return 'master' if PDK::Util.development_mode?
return PDK::TEMPLATE_REF if uri.nil? # The default url always uses PDK::TEMPLATE_REF as the ref
uri = new(uri) unless uri.is_a?(self)
uri.default? ? PDK::TEMPLATE_REF : 'master'
end
There was a problem hiding this comment.
Yeah I think @glennsarti 's guard clause version is clearer.
| @@ -161,7 +161,7 @@ | |||
|
|
|||
| it 'returns the specified template' do | |||
| allow(Gem).to receive(:win_platform?).and_return(true) | |||
| expect(template_uri.to_s).to eq("C:\\cli-templates##{described_class.default_template_ref}") | |||
| expect(template_uri.to_s).to eq('C:\cli-templates#master') | |||
There was a problem hiding this comment.
nit: Although it's not required, I've found just always using double slash makes it far less confusing e.g. \' and \\ is still are valid escape sequences in single quoted strings.
| @@ -302,37 +302,45 @@ | |||
| end | |||
|
|
|||
| describe '.default_template_ref' do | |||
| subject { described_class.default_template_ref } | |||
| subject { described_class.default_template_ref(uri) } | |||
There was a problem hiding this comment.
I don't see any tests for described_class.default_template_ref() anywhere. Should these be added?
There was a problem hiding this comment.
The only thing I would be looking for is:
expect(described_class.default_template_ref()).to eq(PDK::TEMPLATE_REF)
There was a problem hiding this comment.
Overall looks good to me, I like the directness of the test cases.
Agreed with @glennsarti's feedback about simplifying the default_template_ref class method using guard clauses and adding a test for the nil arg case.
| @@ -207,11 +207,14 @@ def self.templates(opts) | |||
| end | |||
|
|
|||
| # @returns String | |||
| def self.default_template_ref | |||
| def self.default_template_ref(uri = nil) | |||
There was a problem hiding this comment.
Yeah I think @glennsarti 's guard clause version is clearer.
No description provided.