search

evilstreak / markdown-js

A Markdown parser for javascript
7.7k stars 863 forks source link

Various #108

Closed XhmikosR closed 11 years ago

XhmikosR commented 11 years ago

I can remove the curly rule addition if you don't agree with it; it's a leftover from my original JSHint patch.

ashb commented 11 years ago

Thanks for tidying this up and picking the commits I'd missed. I mainly wanted to get some form of JSHint in and passing first.

Please remove the curly rule - not really a fan of that one. Looks good after that and I'll get it merged in.

XhmikosR commented 11 years ago

@ashb: How about not using one line statements?

ashb commented 11 years ago

Not fussed either way on one line statements, You've done it so sure why not? :)

XhmikosR commented 11 years ago

I wasn't done yet :(

ashb commented 11 years ago

Oh sorry.

But generally please keep pull requests small and too one thing - it makes it much easier to us to look at them and give feedback.

And if you just want feedback but don't want one merged just yet add a note in the description.

XhmikosR commented 11 years ago

I was waiting for your answer about one line statements.

No big deal.

Here is the final patch https://github.com/XhmikosR/markdown-js/commit/389fc9ac7c1f1f7a573f68bcbc3c1a7b84bc1727

XhmikosR commented 11 years ago

BTW, what do you think about using 4 spaces for indentation? Currently, 2 spaces are mostly used, but there are many cases 4 are used.

Perhaps you could add a CONTRIBUTING.md file with these guidelines.

ashb commented 11 years ago

We're in favour of 2 spaces and yes, a contribution section/file is needed - evilstreak and I were talking about this last night and it should also include things like 'Add a line to the changelog' and 'make sure its tested' etc.

ashb commented 11 years ago

Picked up that last commit too.

XhmikosR commented 11 years ago

4 spaces looks better in my opinion, because it's more consistent in cases like:

var foo = bar,
    fooo = barr;

But it's your call...

ashb commented 11 years ago

Ah so there is indentation (the number of spaces a block is aligned) and alignment (the case you just mentioned).

On the subject of alignment - I'm happy with aligning vars like that, I'm very much against aligning on = like this:

x.foo    = y;
x.longer = z;

because if someone adds a longer property then the diff touches lines that haven't really changed, or you end up with some things aligned and some not:

x.foo    = y;
x.longer = z;
x.even_longer = w;
XhmikosR commented 11 years ago

I agree with not aligning on =.

Using 4 spaces for indentation is simpler/more convenient, I think.