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

Add start_with function #1086

Merged

Conversation

baurmatt
Copy link
Contributor

@baurmatt baurmatt commented Feb 10, 2020

This is helpful in places where you need to use variables in the string
which needs to be compare e.g. something like this:

$test.start_with("part${string}01")

@baurmatt baurmatt requested a review from a team as a code owner February 10, 2020 17:16
@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #1086 into master will increase coverage by 1.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1086      +/-   ##
=========================================
+ Coverage    4.38%   5.41%   +1.03%     
=========================================
  Files         184     185       +1     
  Lines        5362    5246     -116     
=========================================
+ Hits          235     284      +49     
+ Misses       5127    4962     -165
Impacted Files Coverage Δ
lib/puppet/functions/start_with.rb 0% <0%> (ø)
lib/puppet/functions/stdlib/end_with.rb 0% <0%> (ø) ⬆️
lib/puppet/parser/functions/enclose_ipv6.rb 85.71% <0%> (ø) ⬆️
lib/puppet/type/file_line.rb 96.36% <0%> (+1.81%) ⬆️
lib/facter/facter_dot_d.rb 19% <0%> (+5.8%) ⬆️
lib/facter/root_home.rb 69.23% <0%> (+20.65%) ⬆️
lib/facter/puppet_settings.rb 50% <0%> (+20.37%) ⬆️
lib/facter/pe_version.rb 68.75% <0%> (+23.85%) ⬆️
lib/facter/util/puppet_settings.rb 100% <0%> (+44.44%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ade894a...7091a9d. Read the comment docs.

@b4ldr
Copy link
Collaborator

b4ldr commented Feb 12, 2020

wmflib::end_with already exists. and although i much prefer ends_with and starts_with i think it makes more sense to make functions like this, a direct map to the ruby equivalent. i.e. startswith should be renamed to wmflib::start_with

also note you can do the following

$test =~ "^part${string}01"

@baurmatt
Copy link
Contributor Author

Hey @b4ldr, oh missed that oO I will remove the endswith function from the PR.

Do you know when a function which be prefixed with stdlib::? My understanding is, that all modules should prefix their function besides stdlib.

@b4ldr
Copy link
Collaborator

b4ldr commented Feb 12, 2020

Do you know when a function which be prefixed with stdlib::? My understanding is, that all modules should prefix their function besides stdlib.

no im not sure i thought all modules should be prefixed but ill let someone from puppetlabs give a more authoritative answer

@baurmatt baurmatt changed the title Add startswith and endswith function Add startswith function Feb 12, 2020
@baurmatt baurmatt changed the title Add startswith function Add start_with function Feb 12, 2020
@b4ldr
Copy link
Collaborator

b4ldr commented Feb 12, 2020

also the current stdlib::end_with only supports String, it would be nice if you could patch that to also support an Array[String] like your patch

edit: i see you just did thanks :)

@baurmatt
Copy link
Contributor Author

Alright! :) I've updated the PR and tried to adapt the best of both variants :)

The last thing missing seems to be an authoritative answer regarding the function namespacing.

This is helpful in places where you need to use variables in the string
which needs to be compare e.g. something like this:

```puppet
$test.start_with("part${string}01")
```
@daianamezdrea
Copy link
Contributor

Hi @baurmatt ,
Thank you for your contribution !

@daianamezdrea daianamezdrea merged commit 7812314 into puppetlabs:master Feb 14, 2020
@baurmatt
Copy link
Contributor Author

Hey @daianamezdrea, I think there was a decision missing: That's the correct naming for function in stdlib? The implementation that you've just merged had both in it and will, at least, confuse people:

  • stdlib::end_with
  • start_with

@daianamezdrea
Copy link
Contributor

Hi @baurmatt ,
Based on the documentation from here, the naming of the functions is correct, however I do agree with you on this naming confusion, we might need to move the "start_with" function in functions/stdlib/ folder.

Sorry I missed this in the review.

@baurmatt
Copy link
Contributor Author

Hey @daianamezdrea, which naming of the function is correct? Prefixed with stdlib:: or without? The majority of the functions in this module don't prefix it, but some do.

I think I've read somewhere (I can't find the link anymore... -.-) that only stdlib is allowed to skip the module name prefix.

baurmatt added a commit to syseleven/puppetlabs-stdlib that referenced this pull request Mar 4, 2020
This function was originally introduced in puppetlabs#1086 but never got a
decision about the correct namespace. This ensure change ensure
namespace similarity to the end_with function to not confuse people.
baurmatt added a commit to syseleven/puppetlabs-stdlib that referenced this pull request Mar 4, 2020
This function was originally introduced in puppetlabs#1086 but never got a
decision about the correct namespace. This commit adds the stdlib::
namespace to the start_with function after a discussion I had with
Daneel on Slack about this. This will at least align the namespace with
the corresponding end_with function to not confuse people.
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