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

Fix wrong end-tag resulting in blank line. #85

Merged
merged 3 commits into from
Jul 13, 2020

Conversation

chrekh
Copy link
Contributor

@chrekh chrekh commented Jul 12, 2020

No description provided.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙋🏻‍♀️
how is this module currently tested?

how could we regression test changes between module upgrades anyway??

@chrekh
Copy link
Contributor Author

chrekh commented Jul 12, 2020

how could we regression test changes between module upgrades anyway??

Perhaps we could add .without_content(%r{^\s*\n\s*$}) to test for double blank lines?

@chrekh
Copy link
Contributor Author

chrekh commented Jul 12, 2020

Perhaps we could add .without_content(%r{^\s*\n\s*$}) to test for double blank lines?

I tried that, and imediately found an other bug

<% if $chrony::refclocks { -%>

<%   $chrony::refclocks.each |$driver| { -%>
refclock <%= $driver.flatten.join(' ') %>
<%   } -%>
<% } -%>

Is adding a blank line when refclocks is []

@igalic
Copy link
Contributor

igalic commented Jul 13, 2020

you want to test whether that is empty then, or else give it undef as default

@chrekh
Copy link
Contributor Author

chrekh commented Jul 13, 2020

you want to test whether that is empty then, or else give it undef as default

I think we should add type constrain Array to it in init.pp instead. And just remove the if test.

@igalic
Copy link
Contributor

igalic commented Jul 13, 2020

the if controls the insertion of a newline refclock, so the test should be for empty()

@chrekh
Copy link
Contributor Author

chrekh commented Jul 13, 2020

the if controls the insertion of a newline refclock, so the test should be for empty()

And ofcourse you are right. The if is still needed even if we require refclock to be Array. I'll prepare a PR for that.

@igalic
Copy link
Contributor

igalic commented Jul 13, 2020

since this pr is about fixing whitespace, i'd do that here.

@chrekh
Copy link
Contributor Author

chrekh commented Jul 13, 2020

Make sense, ok, I'll do that.

Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add that test you spoke about, i don't see it
then, this should be good to go

chrekh added 2 commits July 13, 2020 15:20
The test `if $chrony::refclocks` was wrong, resulting in a blank line when $refclocks is an
empty array. This tests instead for emptiness.
@chrekh
Copy link
Contributor Author

chrekh commented Jul 13, 2020

Ok, I'll rebase to master (because the added gentoo tests) and add a test for double blank lines. I'll add a separate PR later for restructuring the tests to match the gentoo case.

@igalic igalic merged commit 4601d68 into voxpupuli:master Jul 13, 2020
@chrekh chrekh deleted the fix-empty-line branch July 13, 2020 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants