Added nolinks flag to skip links during rendering #80

debuggio · 2017-03-07T19:57:03Z

pwildani · 2017-03-07T23:36:43Z

fuzzing/snudown-validator.c

@@ -129,6 +136,14 @@ void init_wiki_renderer() {
 	sundown[RENDERER_WIKI].toc_state = &wiki_toc_state;
 }

+void init_default_renderer_without_links(PyObject *module) {
+	PyModule_AddIntConstant(module, "RENDERER_USERTEXT_WITHOUTLINKS", RENDERER_USERTEXT_WITHOUTLINKS);


This version doesn't have a python module to initialize.

debuggio · 2017-03-09T01:07:46Z

💇

lan17 · 2017-03-10T20:02:37Z

snudown.c

@@ -10,6 +10,7 @@
 enum snudown_renderer_mode {


why is this repeated?

repeated where?

Exactly same code is in fuzzing/snudown-validator.c

I don't understand how fuzzer/validator works in this code base, yet, but that seems like a DRY violation?

absolutely agree. But, at least it doesn't register modules, also there are some minor differences that may affect something. I asked Peter about it. His response: "snudown-validator is the snudown.c with a main() function added, so it can be run as a standalone program for fuzz-testing."
I thought about rewriting this part to use validator.c but because I'm not sure how to test it, I decided to leave it as is.

can't we just have a .h file that we include from fuzz and main ?

lan17 · 2017-03-10T20:03:40Z

Can you also describe the business logic of this as well as the reasoning behind this change in the OP?

debuggio · 2017-03-10T22:00:38Z

I've introduced a feature that if subreddit is specific list and someone mentioned user or subreddit in comments for post in this subreddit, users won't receive messages that someone mentioned their name.
Because we assume that this subreddit is spamming we decided to also disable link rendering for this subreddit

lan17 · 2017-03-10T22:06:10Z

This is just the render part, right? Where is the business logic code that decides which subs to use this new renderer for?

debuggio · 2017-03-10T22:07:26Z

https://github.com/reddit/reddit-public/pull/6501

debuggio · 2017-03-11T00:25:13Z

💇
you are right. refactored snudown.c and snudown-validator.c

lan17 · 2017-03-11T00:28:52Z

snudown.h

@@ -0,0 +1,118 @@
+/* snudown.h - header for snudown */


nice. this works just as before for fuzzing as well as using from python?

seems so. I was able to run all tests using python setup.py test

sweet.

AFAIK putting function definitions in .h is not best practice because it gets inlined everywhere in exec, but it should not matter here. Probably someone who knows more about this code base should also weigh in on this decision.

As far as I remember, definition is ok, implementations is not the best practice. I checked other h files and they almost all have function definitions. But agree, if we have some one who has good experience in C, we can ask him\her

While this works for the python module form, it's not well structured and will cause maintenance problems in a few years.

Please put all of the implementations in a separate file. Maybe renderers.c?

The new files will need to be mentioned in fuzzing/CMakeLists.txt to allow snudown-validator to be fuzz tested.

Agree with @pwildani's approach. I really should have put all the shared rendering logic into a shared renderers.c when I first wrote the fuzzing code 😬 .

Also, I don't believe the fuzzer is invoked when calling python setup test. The fuzzing process has a number of extra dependencies (AFL, CMake, the custom Google gumbo bits...) that aren't needed for functional tests. It also doesn't really fit into what people would normally expect python setup.py test to do (you need to run it for multiple days to get trustworthy results.)

I need to update the docs on how to run the fuzzing though, feel free to ping me on Slack about getting it working

Can you guys explain maintenance problems caused by including definition in .h?

Is it because it would be difficult to use different implementation of the functions in case its needed (Such as slightly different implementation in fuzzer code, or somewhere else?)

It's not really a maintenance problem per se, it's just generally considered bad form to have the function implementation itself in the .h instead of the .c. Having the function definition in the .h is OK and generally what you want.

Any function implementation longer than 1-2 lines should be in a .c it only needs to be compiled to a single .o and the implementation won't need to be recompiled for every object with a .c file with #include "snudown.h".

Also, if you put the implementation in the .h rather than the .c, any change to the function implementation will trigger a recompile of any objects that reference snudown.h. If the implementation is somewhere like renderers.c, then a change to the implementation will only require a recompile of that specific object file and a re-link.

#include is dumb and copy pastes code. If you put implementations in headers, they get copy pasted into every module that includes it which means redefinitions.

Thank you for clarification, guys.

pwildani · 2017-03-13T17:49:55Z

👓 @spladug @JordanMilne

I haven't reviewed the current version yet, but the goal is to allow disabling user and subreddit mentions on a per-subreddit basis as a penalty for abusing the feature. Disabling all automatic links might be reasonable fallout.

Butler's notifications are handled in a separate PR.

JordanMilne · 2017-03-17T19:07:50Z

snudown.h

+/* snudown.h - header for snudown */
+
+/*
+* Copyright (c) 2017, Evgenii Timofeev


Nit: We haven't added copyright headers to any of the files added in our fork of Sundown, but copyright should be assigned to Reddit if we do.

JordanMilne · 2017-03-17T19:29:57Z

If the intent is to disable all linking from posts under this new rendering mode we should add testcases to ensure the other forms of links are rendered as plaintext as well:

[foo](/u/bar)
</u/foobar>

JordanMilne · 2017-03-17T20:06:22Z

snudown.c

 #include "autolink.h"
+#include "snudown.h"

 #define SNUDOWN_VERSION "1.4.0"


nit: we should bump this a minor version too, fancy new features!

JordanMilne · 2017-03-17T20:21:39Z

💅 Some questions RE how regular links should behave

debuggio · 2017-03-28T17:20:07Z

💇

pwildani · 2017-03-28T17:40:11Z

src/markdown.c

 	md->active_char['<'] = MD_CHAR_LANGLE;
 	md->active_char['\\'] = MD_CHAR_ESCAPE;
 	md->active_char['&'] = MD_CHAR_ENTITITY;

 	if (extensions & MKDEXT_AUTOLINK) {
 		if (!(extensions & MKDEXT_NO_EMAIL_AUTOLINK))
 			md->active_char['@'] = MD_CHAR_AUTOLINK_EMAIL;
+		if (md->cb.image || md->cb.link)


This change confuses me.

[text](http://url) isn't an automatic link, it's an explicit one the user wrote out, so why would the MKDEXT_AUTOLINK flag affect how that syntax is rendered?

Agreed. We also don't want to skip by the link structure entirely, we just want to not render the link. Outputting [foo](/bar) verbatim probably isn't the way to go either.

I agree that it confuses, but I tried to avoid situations when user is mentioned my direct, without investing much effort. I'll revert this part, so link will be rendered as expected

pwildani · 2017-03-28T17:40:55Z

src/renderers.c

@@ -0,0 +1,98 @@
+#include "markdown.h"


🔕 Exactly right file structure

pwildani · 2017-03-28T17:44:30Z

src/renderers.h

+	struct snudown_renderopt options;
+};
+
+static char* html_element_whitelist[] = { "tr", "th", "td", "table", "tbody", "thead", "tfoot", "caption", NULL };


This is wrong. A static declaration in a header makes a copy of the thing in every compilation unit in which it is included.

Declare the variable as extern here, and define it in the renderers.c, so every reference uses the one compiled into renderers.o

I'll move it to c, also don't think that someone outside renderer should know about it

pwildani · 2017-03-28T17:48:19Z

💅
I have a followup to jordan's question about links.

I realize this is under-defined, but IMO, the syntax is an explicit link and therefore needs a separate flag from the AUTOLINK flag to disable.

I haven't given enough thought to if we should disable all links as a penalty yet, just the automatic ones.

debuggio · 2017-03-28T23:57:02Z

1 thing about links and why I've moved if (md->cb.image || md->cb.link) under if (extensions & MKDEXT_AUTOLINK):
Jordan suggested test case foo. Without my change it is rendered as foo. That I think is what we are trying to avoid. If you think that it's ok to render such links, than I agree that it's more clearer to remove revert this change

debuggio · 2017-03-29T00:37:24Z

💇

JordanMilne

lgtm

pwildani · 2017-04-12T20:48:42Z

🐟

lan17 · 2017-04-14T19:20:57Z

🐟

lan17 · 2017-09-15T22:28:38Z

OMG, this is still open?!

lan17 · 2018-10-19T04:53:09Z

Woah. close this plz@

pwildani reviewed Mar 7, 2017

View reviewed changes

Added nolinks flag to skip links during rendering

3d4cf4c

debuggio force-pushed the evtim/AE-598 branch from 176fcc8 to 3d4cf4c Compare March 9, 2017 01:07

lan17 reviewed Mar 10, 2017

View reviewed changes

Added header for snudown to remove code duplication

d6e2afb

lan17 reviewed Mar 11, 2017

View reviewed changes

JordanMilne reviewed Mar 17, 2017

View reviewed changes

Code review fixes and refactoring

395c8e5

debuggio force-pushed the evtim/AE-598 branch from 90415a9 to 395c8e5 Compare March 28, 2017 17:23

pwildani reviewed Mar 28, 2017

View reviewed changes

src/renderers.c

@@ -0,0 +1,98 @@

#include "markdown.h"

Copy link

pwildani Mar 28, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

🔕 Exactly right file structure

pwildani reviewed Mar 28, 2017

View reviewed changes

minor refactoring in renderers

b2e380a

JordanMilne approved these changes Apr 12, 2017

View reviewed changes

Removed include of unexisting file from snudown-validator

a7375dc

debuggio force-pushed the evtim/AE-598 branch from 605cdcb to a7375dc Compare April 20, 2017 18:32

Fix for snudown-validator

877d9f6

Added nolinks flag to skip links during rendering #80

Are you sure you want to change the base?

Added nolinks flag to skip links during rendering #80

Conversation

debuggio commented Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

debuggio commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debuggio Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lan17 commented Mar 10, 2017

debuggio commented Mar 10, 2017

lan17 commented Mar 10, 2017

debuggio commented Mar 10, 2017

debuggio commented Mar 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordanMilne Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwildani commented Mar 13, 2017

JordanMilne Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordanMilne commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordanMilne commented Mar 17, 2017

debuggio commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwildani Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwildani commented Mar 28, 2017

debuggio commented Mar 28, 2017

debuggio commented Mar 29, 2017

JordanMilne left a comment

Choose a reason for hiding this comment

pwildani commented Apr 12, 2017

lan17 commented Apr 14, 2017

lan17 commented Sep 15, 2017

lan17 commented Oct 19, 2018

debuggio commented Mar 7, 2017 •

edited

Loading

debuggio Mar 10, 2017 •

edited

Loading

JordanMilne Mar 20, 2017 •

edited

Loading

JordanMilne Mar 17, 2017 •

edited

Loading

pwildani Mar 28, 2017 •

edited

Loading