Skip to content

adds new parser called is_absolute_path #553

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

Merged
merged 2 commits into from
Dec 22, 2015

Conversation

logicminds
Copy link
Contributor

I really like the validate_absolute_path function and needed similar functionality to perform conditional logic on installer locations so I created the is_absolute_path function.

This uses the same code as the validate function but only accepts one argument and also returns a value.

@igalic
Copy link
Contributor

igalic commented Dec 8, 2015

@logicminds rather than duplicating the code, i'd now switch validate_absolute_path to calling this function.

@logicminds
Copy link
Contributor Author

@igalic I gave that some serious thought but I just assumed the performance would be better without having to call a puppet function from within a puppet function multiple times. This might be negligible if using once but it folks are using this 50 times across their entire catalog, a 100 ms delay adds up pretty quick. I have no benchmarks to back up my claim though.

@igalic
Copy link
Contributor

igalic commented Dec 8, 2015

so, we have a loop

candidates.each do |path|
  function_is_absolute_path([path]) or raise Puppet::ParseError, ("#{path.inspect} is not an absolute path.")
end

now, a compiler that cannot figure out that this is a hot-spot, and needs optimization probably isn't worth its money, even if it's free.

imo, it's probably cheaper to keep the code in one parser function, than wonder why down the road the two of them are behaving differently.

however: i'm not an stdlib maintainer, so lets wait what @puppetlabs has to say!

@logicminds
Copy link
Contributor Author

Well that is definitely much cleaner.

@logicminds
Copy link
Contributor Author

This PR is only adding a function while most of this conversation is about altering an existing function to rely on a yet to be included function. We need to have a new PR for altering the validation_absolute_path and keep discussing there instead of here.

@bmjen
Copy link
Contributor

bmjen commented Dec 15, 2015

@logicminds I agree with @igalic. For maintainability purposes it would be best to use DRY principles.

Also, I'd be a proponent of coupling the refactoring of validate_absolute_path in this PR if you have the bandwidth to do it. This way the conversation can be cohesive in the why and the when.

@logicminds
Copy link
Contributor Author

ok I'll refactor soon and rebase.

  * is_absolute_path returns boolean true if the given path
    is absolute, returns false otherwise.
  * works for windows and unix
@logicminds
Copy link
Contributor Author

@bmjen this is ready to go.

@bmjen
Copy link
Contributor

bmjen commented Dec 22, 2015

@logicminds Thanks!

@bmjen bmjen removed the needs-docs label Dec 22, 2015
bmjen added a commit that referenced this pull request Dec 22, 2015
adds new parser called is_absolute_path
@bmjen bmjen merged commit 0073c6d into puppetlabs:master Dec 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants