oprypin / crsfml

Crystal bindings to SFML multimedia/game library
https://oprypin.github.io/crsfml
zlib License
350 stars 14 forks source link

[WIP] Feature/restructured architecture #9

Closed paulgoetze closed 9 years ago

paulgoetze commented 9 years ago

I thought it might be good, if the generated code is a bit more tidied up.

My proposal is to move the lib files into their own lib/csfml/ directory and to give them simpler names, like MODULE.cr(without the _lib).

We could do the same with the obj files (move them into sth. like /objects maybe), so that all in all the generated files have their own directories and it's easier to change the other not generated files.

I also made sure that no whitespace is generated.

All changes are of cause applied through the generator. What do you think?

Tasklist:

oprypin commented 9 years ago

Haven't you noticed that I intentionally leave indentation whitespace on empty lines? It's not just generated files. This is how I roll...

The generator code is no man's land anyway, and modifying it against my habits won't help. Plus there are some new changes that are against Python's guidelines anyway: def f(arg=default) shouldn't have spaces.

Moving files into lib folder is also an unnecessary change. There will be pairs of files with the same names, I don't like the potential confusion. Breaks compatibility too.

The changes to .gitignore are incorrect.

All in all, I dislike most of the changes :confused: Too bad that you jumped into doing this without any discussion.

paulgoetze commented 9 years ago

No problem - that's why it is only a proposal.

I must admit I'm not too familiar with the python coding style and conventions, so my changes intended to make the code at least a bit more readable. Anyway, coding style shouldn't be too hard to fix.

I can see your point with the same naming of files. With moving the files I tried to come closer to a usual crystal project structure, where the external lib wrappers are in a separated directory. Still, separation of automatically generated and human written code makes totally sense for me maintenance-wise. I also like to structure my code in different namespaces (i.e. directories, I assume that Crystal uses a similar project structure to Ruby projects?) which is kind of hard to do here, because the audio, graphic etc. functionality should all be available under the SF namespace. It might still be possible to combine e.g. the graphics things under a Graphics namespace/module and include and alias the classes afterwards in the SF module. Could you explain how you think this would break compatibility?

However, I can't really see the advantages of using whitespace. In my point of view whitespace is always a potential source of unnecessary changes if you remove or add it unintentionally. But I guess whitespace is something one could argue about :)

Anyway - thanks for the quick response and thumbs-up for the project!

oprypin commented 9 years ago

Yes, it is true that usually whitespace is undesired. It just annoys me when I want to edit the code and sometimes my cursor jumps to the very left side... Maybe I could agree about removing whitespace in *.cr files.


About breaking compatibility I meant usage like require "crsfml/audio_lib".

About restructuring -- I don't like this kind of separation. This would make the most sense to me:

About sub-namespaces - I would actually quite like it (especially for the purposes of documentation), but only if everything would also be available under the main namespace.

Both of these things may be problematic to implement.


Another problem is CrSFML is changing a lot lately. Maybe worth waiting with this? The end of these changes is near.

In summary, good things are:

Unacceptable things:

paulgoetze commented 9 years ago

Alright, sounds like a plan. I think it's worth opening separate issues for "Adding header location arg" & "Remove trailing whitespace", so that we can keep track of them. If you like I can have a look at these again.

Then let's wait with the restructuring until the major changes are done. Just ping me, if I can help with this in any way. :bell:

I think then this PR can also be closed for now.

oprypin commented 9 years ago

Is this OK with you? (partial commits)

https://github.com/BlaXpirit/crsfml/commits/whitespace-changes https://github.com/BlaXpirit/crsfml/commits/a064069b2fc415cb4779ff8c036b153cc4523c1c

paulgoetze commented 9 years ago

:+1: Sure. Looks good, thanks.