Skip to content

diffie-hellman: add test template #2203

Merged
cmccandless merged 6 commits into
exercism:masterfrom
pyropy:diffie-hellman-add-test-template
May 28, 2020
Merged

diffie-hellman: add test template #2203
cmccandless merged 6 commits into
exercism:masterfrom
pyropy:diffie-hellman-add-test-template

Conversation

@pyropy
Copy link
Copy Markdown
Contributor

@pyropy pyropy commented May 11, 2020

-Add jinja2 test template for Diffie Hellman exercise #1944

  • Split to_snake into two functions (to_snake_base and to_snake)
  • Register to_snake_base jinja2 macro
  • Generate new test template

Split to_snake into two functions
Register to_snake_base macro
@pyropy pyropy requested a review from a team as a code owner May 11, 2020 12:56
Copy link
Copy Markdown
Contributor

@yawpitch yawpitch left a comment

Choose a reason for hiding this comment

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

Well done on the template, but it's not exactly obvious from the docstrings that to_snake_base isn't converting whitespace and punctuation, while to_snake is ...

Consider instead something like:

def to_snake(string, wordchars_only=False):
    """
    Converts pretty much anything to snake case.

    By default whitespace and punctuation will be converted to underscores as well, pass wordchars_only=True to preserve these as is.
    """
   [...]

Then only the one filter is required and when needed it can be called with | to_snake(wordchars_only=True)?

@pyropy
Copy link
Copy Markdown
Contributor Author

pyropy commented May 12, 2020

Well done on the template, but it's not exactly obvious from the docstrings that to_snake_base isn't converting whitespace and punctuation, while to_snake is ...

Consider instead something like:

def to_snake(string, wordchars_only=False):
    """
    Converts pretty much anything to snake case.

    By default whitespace and punctuation will be converted to underscores as well, pass wordchars_only=True to preserve these as is.
    """
   [...]

Then only the one filter is required and when needed it can be called with | to_snake(wordchars_only=True)?

Yeah I'll try that out. Wasn't quite sure how would that work with Jinja2

Remove to_snake base
If wordchars_only argument is True to_snake function will not replace
punctuation and whitespace characters with underscore
Copy link
Copy Markdown
Contributor Author

@pyropy pyropy left a comment

Choose a reason for hiding this comment

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

I've deleted to_snake_base function and added suggested changes

@pyropy pyropy requested a review from yawpitch May 12, 2020 13:41
Copy link
Copy Markdown
Contributor

@yawpitch yawpitch left a comment

Choose a reason for hiding this comment

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

That's great, keeps the generator itself much cleaner. I've flagged a couple of small additional changes, now that I've had a chance to look more closely at the template.

Comment thread exercises/diffie-hellman/.meta/template.j2 Outdated
Comment thread exercises/diffie-hellman/.meta/template.j2
Copy link
Copy Markdown
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

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

A great first pass!

Comment thread exercises/diffie-hellman/.meta/template.j2 Outdated
Comment thread exercises/diffie-hellman/.meta/template.j2 Outdated
Comment thread exercises/diffie-hellman/.meta/template.j2
Comment thread exercises/diffie-hellman/diffie_hellman_test.py
Comment thread exercises/diffie-hellman/diffie_hellman_test.py
Comment thread exercises/diffie-hellman/diffie_hellman_test.py
Add comment to random private key test
Replace for loop with list comphrenhension
@pyropy pyropy requested review from cmccandless and yawpitch May 12, 2020 18:29
Copy link
Copy Markdown
Contributor

@yawpitch yawpitch left a comment

Choose a reason for hiding this comment

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

With the resolutions to @cmccandless comments I'd say this is now good, but I'll wait for a second before merge.

@cmccandless cmccandless merged commit b4bc89a into exercism:master May 28, 2020
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