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

Add support to print Markdown files with underscored variable names escaped (#48) #63

Merged
merged 7 commits into from
Oct 24, 2018

Conversation

sjauld
Copy link
Contributor

@sjauld sjauld commented Sep 25, 2018

Note: this will cause a conflict with #43 but I'll rebase whichever one needs it...

Prerequisites

Put an x into the box(es) that apply:

  • This pull request fixes a bug.
  • This pull request adds a feature.
  • This pull request introduces breaking change.

For more information, see the Contributing Guide.

Description

Adds a flag to escape underscored variables / outputs when printing markdown.

Issues Resolved

#48

Checklist

Put an x into all boxes that apply:

Tests

  • I have added tests to cover my changes.
  • All tests pass when I run make test.

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Code Style

  • My code follows the code style of this project.

@metmajer
Copy link
Member

metmajer commented Oct 9, 2018

Hi @sjauld and thanks for another valuable contribution! There are some things we should adjust, and I'll get back to you with feedback very soon!

@metmajer
Copy link
Member

metmajer commented Oct 9, 2018

@sjauld can you please do a git merge on your branch so that we can start the review. Let me know if you need help.

@sjauld
Copy link
Contributor Author

sjauld commented Oct 9, 2018

Yes, I'll rebase today.

@sjauld
Copy link
Contributor Author

sjauld commented Oct 11, 2018

Should be good to go now!

@metmajer
Copy link
Member

metmajer commented Oct 11, 2018

Thanks, @sjauld. Here's some feedback, let's discuss!

  1. You've created an option --escape-underscores, but should we really design proper escaping as a feature? Here's how I see this: if a user creates a variable with a _, he or she doesn't want it to show up as format change in the output, but as what it is. Hence, I strongly suggest you remove the option again and treat this as a bug fix.
  2. In your variables.tf and your tests, please rename the variable from string_number_3 to input_with_underscores to reflect it's purpose. Also, please make sure to keep the variable names in these files in the intended order (we do follow an ascending or descending sort order depending on the use-case). This is intended to help upcoming contributions and generally, make insertions easier and the look more consistent.

As a result of these changes...

  • escapeUnderscoresIfSet can be removed and its call can be replaced with the call to strings.Replace
  • markdown-WithEscapedUnderscores.golden can be removed, as input_with_underscores will already be tested in all other .golden files, since we're creating a fix, not an optional feature.

Overall, this would greatly simplify things. Cheers!

@metmajer metmajer changed the title Add support to print Markdown files with underscored variable names escaped (#48) WIP: Add support to print Markdown files with underscored variable names escaped (#48) Oct 18, 2018
@sjauld
Copy link
Contributor Author

sjauld commented Oct 23, 2018

@metmajer this approach seems sensible, I'll try to close out today.

@metmajer
Copy link
Member

No worries, @sjauld, we all have other things to do. Thanks for staying at it 👍

@sjauld sjauld changed the title WIP: Add support to print Markdown files with underscored variable names escaped (#48) Add support to print Markdown files with underscored variable names escaped (#48) Oct 23, 2018
@sjauld
Copy link
Contributor Author

sjauld commented Oct 23, 2018

Ok should be good to go now!

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.

None yet

2 participants