search

OtherCrashOverride / go-play

Retro emulation for the ODROID-GO
https://www.hardkernel.com/shop/odroid-go/
218 stars 72 forks source link

[Proposal] Tab vs Space #4

Open rickyzhang82 opened 6 years ago

rickyzhang82 commented 6 years ago

I saw your code mixed with spaces and tab.

Shall we unify the coding style? Either tab or x/y/z # of spaces.

OtherCrashOverride commented 6 years ago

Each emulator originates from a different source with different authors and contributors (all copyright notices are preserved). When introducing new code, I use tabs but sometimes the editor (Atom) makes up its own mind about tab, spaces, and line feeds.

rickyzhang82 commented 6 years ago

I see. I'd prefer to use 4 spaces. Let's pick one and stick with it. We can run sed to replace them once for all. The formatting doesn't look good in my Qt Creator.

OtherCrashOverride commented 6 years ago

My current focus is on the code itself. Once the code base has stabilized, I will be more amicable to cosmetic changes. I feel that currently the commit 'noise' from it would be disruptive.

rickyzhang82 commented 6 years ago

Coding style is important in collaboration. Since there is no PR yet, it is a right time to have the good foundation to start with. I'd recommend you to get this done ASAP. It will be more disruptive to do this later than now.

I myself is working on improving reduce frame buffer traffic in SPI. I may send your a PR if I got an improvement.

rickyzhang82 commented 6 years ago

I got the original nofredeno source code. It uses 3 spaces as indentation.

It must be either you or someone introducing mixed space and tab when porting nofredeno into ESP32.

If you allow, I can send you a PR to fix it once for all.

OtherCrashOverride commented 6 years ago

Currently, I do not have time to review a PR for cosmetic changed. In C/C++ white space can be syntactic with if/for statements. Changes to white space will need to be reviewed to ensure they do not alter code flow inadvertently.

I recommend creating the PR and hopefully someone else can review it.

rickyzhang82 commented 6 years ago

Please fix your editor first and stick with whatever indention your like.

Your editor introduced indentation mixed with 4 spaces and tab in file nesemu-go/components/nofrendo-esp32/video_audio.c.

In C/C++, incorrect white space in indentation wont't cause syntax error like Python. It is not a deal breaker. But it is irritating and doesn't show professionalism.

kamotswind commented 6 years ago

@rickyzhang82 I think this is an extreme waste of time right now. If you want it better, submit a pull request with your changes. Complaining about something as trivial as white space differences is obnoxious.

White space can be easily corrected later.

rickyzhang82 commented 6 years ago

A sed command can get this done easily. But it is better we can have a clean slate sooner.

If you don't send any new feature PR at all and has no conflict later with white space difference here and there, for sure it is not waste of anyone's time. The merge is a nightmare. That's based on my experiences in Github and at work.

I'm busying studying ESP32 and working on reducing tearing screen in NES. I already started some works on my repo. I hated the fact that maintainer doesn't take it seriously and unwilling to make a decision.

OtherCrashOverride commented 6 years ago

I agree that it should be done. I just have higher priority tasks to complete at the current time. A white space PR would be disruptive to active development already in progress. Therefore, I feel its best to wait for the development to stabilize first

rickyzhang82 commented 6 years ago

I myself may send your a PR if I can improve tearing. I don't want to mess with white space when merge it back.

I totally understand your situation. I hope you can get this done otherwise, it might cause a headache to whoever wants to send you a PR.

Cwiiis commented 5 years ago

I guess this doesn't need much more conversation, but for the record, I'm also working on some bits and the white-space issue is making things difficult. It's harder than it needs to be to read through changes when each change also includes unrelated whitespace changes.

Something to consider (or not), every large open-source project I've worked on is very strict about this, and reject changes when they aren't consistent with the existing coding style. It's fine that each emulator has a different style, but the changes on top of them are adding inconsistency and making them more difficult to contribute to. This could well be turning less driven/vocal people off. Unlikely, but possible.