ohai / ruby-sdl2

A Ruby wrapper for SDL 2.x
GNU Lesser General Public License v3.0
71 stars 16 forks source link

Modernize project structure #3

Closed furunkel closed 8 years ago

furunkel commented 8 years ago

This PR modernizes the project structure.

Among other things:

The gem name is now changed to 'sdl2', as it should reflect the directory and module structure (which is sdl2 and SDL2, respectively.

ohai commented 8 years ago

Thank you for your PR.

First of all, m4 is used because yard does not understand the C macros. Now the code processed by m4 is used to generate documents. Probably this change breaks the document. How about that?

furunkel commented 8 years ago

It does, yes. But as far as I can tell only the keycodes are affected. This is a sacrifice well worth making, considering that those codes are self-explanatory anyway.

ohai commented 8 years ago

The change affects not only the doucmentation of keycode constants. All m4 macros are used for the documentation of constants. For example, the change also affects SDL2::Mixer constants.

furunkel commented 8 years ago

Really ? How so ? The only two m4 macro occurrences I spotted are: /* define(DEFINE_MIX_INIT',rb_define_const(mMixer, "INIT_$1", UINT2NUM(MIX_INIT_$1))') */ and /* define(DEFINE_MIX_FORMAT',rb_define_const(mMixer, "FORMAT_$1", UINT2NUM(AUDIO_$1))') */

ohai commented 8 years ago

Do you run yard by yourself? I don't find the way to run yard from rake after applying your change.

For example your change drops the documentation of SDL2::Mixer::INIT_OGG.

furunkel commented 8 years ago

Yard task is now there. Although it prints quite some warnings and errors.

ohai commented 8 years ago

Now I try the new yard task. As I mentioned above, many documents for constants such as SDL2::Mixer::INIT_OGG are removed when I apply your changes.

I feel it is difficult to accept your PR as it is.

furunkel commented 8 years ago

Well, that makes absolutely no sense to my mind. Why would they disappear ?

I feel it is difficult to accept your PR as it is.

Nop, I can keep this in my fork.

ohai commented 8 years ago

Yard recognizes the following C code as the definition of SDL2::Mixer::INIT_OGG:

   /* Initialize Ogg vorbis loader */
  rb_define_const(mMixer, "INIT_OGG", UINT2NUM(MIX_INIT_OGG));

But yard does not recognize the following C code:

#define DEFINE_MIX_INIT(c) \
  rb_define_const(mMixer, "INIT_" #c, UINT2NUM(MIX_INIT_## c))
/* Initialize Ogg vorbis loader */
DEFINE_MIX_INIT(OGG);

because yard cannot understand the C macros.

This is because because your changes fail to handle the constant documentation and I use the m4 macros instead of C macros.

furunkel commented 8 years ago

Now I see. Too bad. I will revert this part and add the necessary m4 translation task, might take a day or two, though.

ohai commented 8 years ago

Good.

Comments for your PR:

furunkel commented 8 years ago

Okay, the vexatious m4 files are back in place, there is just no way around them. Note that the C files are staged so that m4 becomes a mere development dependency. .m4 files are automatically translated before compilation. YARD documentation generation seems to work too. Can't do more than this.

You are, of course, free to change anything you don't like.

furunkel commented 8 years ago

I forgot using a separate feature branch, I'd like to do further changes in my master branch, so I'm going to close this now. Feel free to merge in the commits up to this.