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

Reddit slash links #32

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

jimmyjxiao
Copy link

@jimmyjxiao jimmyjxiao commented Jan 8, 2018

This implements an option for Reddit subreddit and user links such as /r/test or r/test. Sorry about my complete lack of skill with git

@codecov
Copy link

codecov bot commented Jan 8, 2018

Codecov Report

Merging #32 into master will decrease coverage by 0.03%.
The diff coverage is 91.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   92.75%   92.71%   -0.04%     
==========================================
  Files           1        1              
  Lines        2525     2567      +42     
==========================================
+ Hits         2342     2380      +38     
- Misses        183      187       +4
Impacted Files Coverage Δ
md4c/md4c.c 92.71% <91.3%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0002e2...c959ac7. Read the comment docs.

Copy link
Owner

@mity mity left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

But I miss one important thing before considering merging this, and that is adding some tests. Please add some tests covering the new feature into a new file under test/[something].txt and update scripts/run-tests.sh accordingly.

@@ -288,6 +293,7 @@ cmdline_callback(int opt, char const* value, void* data)

case 'c': parser_flags = MD_DIALECT_COMMONMARK; break;
case 'g': parser_flags = MD_DIALECT_GITHUB; break;
case 'R': parser_flags = MD_DIALECT_REDDITPOST; break;

Copy link
Owner

Choose a reason for hiding this comment

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

It's a minor detail, but please swap 'r' and 'R' so that there is some consistency: Lower case for dialect options, upper case for extensions.

@@ -302,6 +308,7 @@ cmdline_callback(int opt, char const* value, void* data)
case 'V': parser_flags |= MD_FLAG_PERMISSIVEAUTOLINKS; break;
case 'T': parser_flags |= MD_FLAG_TABLES; break;
case 'S': parser_flags |= MD_FLAG_STRIKETHROUGH; break;
case 'r': parser_flags |= MD_FLAG_REDDITSLASHDETECTION;
Copy link
Owner

Choose a reason for hiding this comment

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

Missing break; here, isn't it?

md4c/md4c.c Outdated
break;
}

{
Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove the bogus blank lines in the code below?

README.md Outdated
@@ -104,6 +104,8 @@ some extensions or allowing some deviations from the specification.

* With the flag `MD_FLAG_NOINDENTEDCODEBLOCKS`, indented code blocks are
disabled.

* With the flag `MD_FLAG_REDDITSLASHDETECTION`, Reddit subreddit and user links such as r/test or /u/me are detected
Copy link
Owner

Choose a reason for hiding this comment

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

Please rename the flag for consistency with other flags. No other flag uses the word "detection". I am not reddit user, so IDK whether something like MD_FLAG_REDDITAUTOLINKS but it could give you an idea.

md4c/md4c.c Outdated
@@ -2696,7 +2696,8 @@ md_build_mark_char_map(MD_CTX* ctx)
ctx->mark_char_map['!'] = 1;
ctx->mark_char_map[']'] = 1;
ctx->mark_char_map['\0'] = 1;

if (ctx->r.flags & MD_FLAG_REDDITSLASHDETECTION)
Copy link
Owner

Choose a reason for hiding this comment

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

Uff, indentation using tab chars here and on some other places of your patch. Please use just spaces as in the rest of the source file(s).

md4c/md4c.h Outdated
@@ -271,6 +279,8 @@ typedef struct MD_SPAN_IMG_DETAIL {

#define MD_FLAG_PERMISSIVEAUTOLINKS (MD_FLAG_PERMISSIVEEMAILAUTOLINKS | MD_FLAG_PERMISSIVEURLAUTOLINKS | MD_FLAG_PERMISSIVEWWWAUTOLINKS)
#define MD_FLAG_NOHTML (MD_FLAG_NOHTMLBLOCKS | MD_FLAG_NOHTMLSPANS)
#define MD_FLAG_REDDITSLASHDETECTION 0x8000 /* Enable Reddit autolinks */
#define MD_FLAG_REDDIT_SLASHES_AS_LINKS 0x4000 //Instead of making Reddit links into special spans, make them into web links
Copy link
Owner

Choose a reason for hiding this comment

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

I think that the logic related to the MD_FLAG_REDDIT_SLASHES_AS_LINKS has nothing to do with the parsing but only with output rendering, and therefore it would be IMHO better to place the related logic just into the md2html utility. Take a look how it treats e.g. MD_RENDER_FLAG_VERBATIM_ENTITIES.

Or is there something what I have overlooked?

@jimmyjxiao
Copy link
Author

I think I've fixed most of your concerns, can you take another look?

@mity
Copy link
Owner

mity commented Jan 10, 2018

I think I've fixed most of your concerns, can you take another look?

There is yet the problem with the blank lines between lines 4010 and 4045 in the file md4c.c which were introduced somewhere in your branch so please fix that too.

I will yet do some tests which can take few days before merging it, but so far it seems in quite a good shape to me.

@mity
Copy link
Owner

mity commented Jan 10, 2018

There is yet another problem: The new tests did not run correctly. See https://travis-ci.org/mity/md4c/builds/326630844?utm_source=github_status&utm_medium=notification (you may need to open the triangle mark on line 553 of the log to see the output of the tests).

EDIT: The culprit is in scripts/run-tests.sh:

$PYTHON "$TEST_DIR/spec_tests.py" -s
"$TEST_DIR/reddit-autolinks.txt" -p "$PROGRAM --freddit-autolinks"

The end-of-line generally means end of command, so shell sees this as two commands, not one with more options. So concatenating the two lines should solve it.

@jimmyjxiao
Copy link
Author

Alright, should be fixed

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