Skip to content

Support whitespace in function decls#5

Closed
jdarling wants to merge 3 commits intoreconquest:masterfrom
jdarling:master
Closed

Support whitespace in function decls#5
jdarling wants to merge 3 commits intoreconquest:masterfrom
jdarling:master

Conversation

@jdarling
Copy link
Copy Markdown

Addresses #4

The current pattern doesn't allow for whitespace in function decls, this adds some basic support.

Copy link
Copy Markdown

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I'm not sire whether capturing groups are nessesary but this looks like a nice addition.

Maybe we could add some testcases somewhere to the project to verify the entire code still works (in future updates).

@jdarling
Copy link
Copy Markdown
Author

That would be a good idea. Using this at Pearson for our internal TechOps shell scripts, might have a look at adding in tests while I'm adding in tests to our stuff as well. My guess would be that your preference is to use tests.sh?

@jdarling
Copy link
Copy Markdown
Author

As for the capture groups, I'm used to building lexers so they are automatic in my brain for whitespace. Probably not necessary.

@seletskiy
Copy link
Copy Markdown
Member

@jdarling: Maybe use \s instead of [ \t]?

I guess, with \s it will be more readable without capture groups (note that nasty \(( pattern).

@seletskiy
Copy link
Copy Markdown
Member

@jdarling: Can you take a look at my comment so I can merge your PR?

@jdarling
Copy link
Copy Markdown
Author

Sorry I completely missed your comment. Switched over and seems to work just fine with all the cases I have in our docs. Unfortunately this also brings in a change to allow @name usage (needs readme update) for the case where we are wanting to document a unit instead of a function.

@constrict0r constrict0r mentioned this pull request Aug 30, 2018
@kovetskiy
Copy link
Copy Markdown
Member

Solved in #8.

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.

4 participants