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

ISSUE-151 - Add Charset type for GitHub markdown formatting: #163

Conversation

jmcconnell26
Copy link
Contributor

  • Add new accepted charset value to enum
  • Remove any colouration for GitHub charset
  • Wrap the report written to the README in backticks to ensure
    consistent formatting

Signed-off-by: joshmc [email protected]

@anderejd
Copy link
Contributor

anderejd commented Dec 11, 2020

Looks good to me. Can we use the UTF-8 with emoji output mode? Should work fine for a readme markdown file?

EDIT 1:
...strike that. It seems like terminal control codes are getting written to the readme output. I'm using macOS, do you see the same issue in your dev environment?

EDIT 2:
The alignment of the tree table is broken in this branch, seems correct on master for me.

@anderejd
Copy link
Contributor

Another related idea, #10

Copy link
Contributor

@anderejd anderejd left a comment

Choose a reason for hiding this comment

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

See issues in my previous comments above.

@jmcconnell26 jmcconnell26 force-pushed the ISSUE-151-AddGitHubMDFormatting branch from db01f98 to 91594c9 Compare December 13, 2020 08:46
@jmcconnell26
Copy link
Contributor Author

Apologies, I hadn't meant to close this (I'd just rebased the latest master into these changes, which actually makes sense if changes had been requested). I don't look to have the ability to reopen this PR, shall I open a new one for the requested changes?

@anderejd
Copy link
Contributor

Github says "There are no new commits on the jmcconnell26:ISSUE-151... branch" when I hover the reopen button. Try to push a new commit. Btw, there's no need to rebase and squash, having multiple small commits tends to make review easier. Huge PRs can be nice to squash at the end of review before merging though.

@jmcconnell26
Copy link
Contributor Author

I think I may have messed up the git history, I'll pull it together in a new commit.
Just to check about the issues you are seeing, are you seeing the terminal commands written when using the GitHubMarkdown charset? I left the terminal commands in for the other charsets (to allow people to use them in READMEs not hosted on GitHub, and only likely to be read from a terminal setting).

@anderejd
Copy link
Contributor

anderejd commented Dec 13, 2020

Just to check about the issues you are seeing, are you seeing the terminal commands written when using the GitHubMarkdown charset?

I did not specify a charset, so the default one.

I left the terminal commands in for the other charsets (to allow people to use them in READMEs not hosted on GitHub, and only likely to be read from a terminal setting).

I've never seen terminal control codes (except the normal newlines etc.) stored in readme files, it seems strange to me. Let's remove all special terminal codes that cannot be converted to the target output format.

@jmcconnell26
Copy link
Contributor Author

@anderejd, I've opened a new PR here #164 and made the changes to force the README update to be generated with the terminal codes removed.
I've also caught the error that caused the issues you were seeing with the terminal output on the branch, and fixed in the PR.
Please let me know if there's anything else you would like me to change!
(Also apologies for the slow replies over the last few days, hopefully should be better after this week!)

Thanks,
Josh

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