Skip to content

Make type aliases stricter #1066

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 4 commits into from
Feb 7, 2020

Conversation

pegasd
Copy link
Contributor

@pegasd pegasd commented Nov 15, 2019

This PR makes the following changes:

  • Start matching beginning and end of string instead of line (\A and \z instead of ^ and $)
  • Add initially failing specs that pass with the changes
  • Add specs for Stdlib::MAC type that were absent
  • Leave the Stdlib::Compat::* types intact for compatibility purposes (is this correct behavior? or should I update those as well?)

@pegasd pegasd requested a review from a team as a code owner November 15, 2019 12:53
@pegasd
Copy link
Contributor Author

pegasd commented Nov 18, 2019

Let me elaborate a bit on that last point.

The Stdlib::Compat::* types emulate behavior of functions that reside in lib/puppet/parser/functions and users are discouraged from using them as they are considered to be legacy types.

However, I found out that Stdlib::Host uses Stdlib::Compat::Ip_address as one of the variants and that type looks like fair play.

So, the dilemma I have is whether we should update those types to be more restrictive and therefore more useful, but at the same time break compatibility? Maybe compatibility is overrated in this particular case?

@florindragos
Copy link
Contributor

Hello @pegasd
Thanks for your contribution!
The fix looks good so far.
I assume you encountered issues where you had new lines and the regex did not match those. This would only break compatibility if someone was expecting it to not match new lines.
I'm not sure about the Stdlib::Compat::* types issue. Did you encounter a case where it is breaking compatibility?

@pegasd
Copy link
Contributor Author

pegasd commented Nov 28, 2019

Hey there, @florindragos !
The main issue that I'm trying to solve with this PR is this: while one expects that Stdlib::Fqdn, for example, matches things like

example.com

in reality it also matches a multiline string like

RANDOM TRASH {}[]!!
example.com

more stuff
👌🏻

@pegasd
Copy link
Contributor Author

pegasd commented Nov 28, 2019

By breaking compatibility I mean that if I update the Stdlib::Compat::* types, they will not match the behavior of corresponding functions.

I guess I could update those as well, but as I said, they're very much legacy and things may break for users in unexpected ways.

What do you propose? I can take a stab at compat if needed.

@pegasd
Copy link
Contributor Author

pegasd commented Jan 29, 2020

Anything I can do to make this PR move along?

@carabasdaniel
Copy link
Contributor

Hi @pegasd,
Thank you for your contribution, this looks good.

@carabasdaniel carabasdaniel merged commit 6db9ba9 into puppetlabs:master Feb 7, 2020
@pegasd pegasd mentioned this pull request Feb 10, 2021
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