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

Finish reorganization #9

Merged
merged 19 commits into from
Sep 21, 2013
Merged

Conversation

mildsunrise
Copy link
Member

Now that @devinus has done the base work,
there's still some things to do, namely:

Code (3)

  • Prefix enums as well. Currently they start with
      MKD_ or HTML_.
  • Normalize guard names and comments on headers.
  • General cleanup.

Building and versioning (4)

  • Remove the html/ directory from Makefiles.
  • Makefile.win should be modified as well.
  • Add everything to hoedown.def.
  • Reset version.

Readme and licensing (5)

  • Correct a typo at README.
  • Review the README. Especially clarify the "bindings" part:
    all those bindings currently aren't Hoedown bindings.
  • Rewrite the "Install" section. The "it's just three files" part
    is not true anymore.
  • Does README's License match with LICENSE?
  • Update preambles where necessary.

@mildsunrise
Copy link
Member Author

I'm creating a wiki page to list the [future] bindings, and referencing it from README.

@devinus, you should also change the "Help us" section, I don't know what to put in there.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

markdown.{c,h} should be left as is. This is because its name expresses what part of Sundown/Hoedown is (in that case, the Markdown parser). Hoedown is comprised of all the sources, not just the Markdown parsing part.
Same for buffer.{c,h}. We want to keep things simple, so if the user wants to group Hoedown sources he can just put them in a hoedown folder, and then #include "hoedown/markdown.h" (to use the Markdown parser) or #include "hoedown/escape.h" (to use just the houdini part).

I agree. I changed my mind last night too.

Prefix enums as well. Currently they start with MKD_ or HTML_

Agreed. The only reason I didn't do it last night is because I wanted to figure out what to name the MKD_EXT_* values. HOEDOWN_MKD_EXT_* is a little too long. I for sure want them prefixed with HOEDOWN_* but at the minimum they should become HOEDOWN_MD_EXT_* or HOEDOWN_EXT_*. HOEDOWN_EXT_* is currently winning me over since some extensions aren't even general Markdown extensions you see elsewhere, just stuff that the Redcarpet team liked.

@mildsunrise
Copy link
Member Author

I'm totally going for the HOEDOWN_EXT_* choice.
And for the HTML_* ones, HOEDOWN_HTML_*.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

👍

@mildsunrise
Copy link
Member Author

Also I'm going to remove most of the "Credits" section;
it's mostly incomplete and it doesn't belong there.

@mildsunrise
Copy link
Member Author

Can I remove the preambles on Makefile and examples/?
I suppose so.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

Please do.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

Can you also update the LICENSE to say:

Copyright (c) 2013, Devin Torres and the Hoedown authors

Or I can do it later tonight.

@mildsunrise
Copy link
Member Author

I will do it.

@mildsunrise
Copy link
Member Author

Done, only two tasks left.

@mildsunrise
Copy link
Member Author

I did it! I managed to refactor every enum!
It builds correctly. Look at the three commits.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

Excellent! I plan to benchmark the branch prediction Houdini added later today and see if it's worth it.

@mildsunrise
Copy link
Member Author

Goodie! 😃

@mildsunrise
Copy link
Member Author

One thing I'd also like to do is to reformat the code
to a two-space indentation, as the Google Style
Guide says.

Currently there's a mix of tabs, 8 spaces and 4 spaces,
sometimes even on the same file.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

@jmendeth There's only a few places where spaces are used (I did a grep -R " "). We could clean those up, but I'm not sure about changing the indentation at this point.

Rationale: While I hate tab indentation (seriously, who still uses tabs?) and love me some two space indentation, we would clobber all git blame functionality forever. :(

@mildsunrise
Copy link
Member Author

Uhm, yeah, I understand you.
But if you think about it, we have already clobbered it
with all these backporting, name changing, cleanup, etc.

@mildsunrise
Copy link
Member Author

But if you don't want, that's fine, it's not a must-have for me.

I have to go to the bed (or my parents'll kill me)
so I'll finish this PR tomorrow.

@devinus
Copy link
Member

devinus commented Sep 20, 2013

Well, thankfully the name changing would only affect lines that used an exposed interface. I'm more concerned with blocks like this:

            if (i < lang->size) {
                size_t org = i;
                while (i < lang->size && !isspace(lang->data[i]))
                    i++;

                if (lang->data[org] == '.')
                    org++;

...which hasn't been clobbered yet, and if it came from a backport we have a clear history on it since I took the time to keep it.

We can and should discuss coding style though, but definitely in another issue.

@mildsunrise
Copy link
Member Author

No, I definitively don't want to spend time debating this aspect. ;)
At least for now. Good night!

@devinus
Copy link
Member

devinus commented Sep 21, 2013

Is this ready?

@mildsunrise
Copy link
Member Author

All ready! It includes your latest changes
and it builds without a warning.

devinus pushed a commit that referenced this pull request Sep 21, 2013
@devinus devinus merged commit bdefe91 into hoedown:master Sep 21, 2013
@mildsunrise mildsunrise deleted the misc-reorganize branch September 21, 2013 08:29
@mildsunrise
Copy link
Member Author

Weeeee!

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