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

substitute multiple '%pistol-extra%' in one argument #58

Merged
merged 1 commit into from
Apr 19, 2021
Merged

substitute multiple '%pistol-extra%' in one argument #58

merged 1 commit into from
Apr 19, 2021

Conversation

lucas-mior
Copy link
Contributor

adresses #56
I don't think it's an elegant solution, but it should work.

@doronbehar
Copy link
Owner

Thanks for trying to fix this @lucas-mior . I added a test to your branch, which demonstrates the issue and it seems it's not fixed completely (run make test):

Checks substitution of multiple pistol-extra arguments without
a space between them (issue 56). The output should be:

     tests/multi-extra AxB

-------------------
tests/multi-extra %pistol-extra0%xB

Without your change to previewer.go the test fails even worse:

tests/multi-extra

So this is a good start.

@lucas-mior
Copy link
Contributor Author

Weird, running make test in my machine results in

...
-------------------
Checks substitution of multiple pistol-extra arguments without
a space between them (issue 56). The output should be:

     tests/multi-extra AxB

-------------------
tests/multi-extra AxB

Add a test to demonstrate the issue, and fix it.
@doronbehar
Copy link
Owner

Weird, running make test in my machine results in

Weird indeed, perhaps I forgot to run make after I tested some changes.. Thanks for the fix! Could you please review the comments I added by reading your code? If you'd like to do / suggest changes please squash the commits.

@lucas-mior
Copy link
Contributor Author

The comments look fine to me.

@doronbehar doronbehar merged commit ac92b11 into doronbehar:master Apr 19, 2021
doronbehar pushed a commit that referenced this pull request Apr 20, 2021
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.

2 participants