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

Organized source file structure, Removed unused SM5 files, Moved libraries #455

Merged
merged 16 commits into from
Mar 12, 2019

Conversation

jameskr97
Copy link
Member

@jameskr97 jameskr97 commented Mar 7, 2019

This is my first pull request on this repository.

Here is a summary of everything changed in this pull request.

  1. (Largest change) The src folder with every source file has been refactored to match the Visual Studio filters as defined in the src/CMakeData-XXXX.cmake files. Mostly a QOL commit so contributors not using visual studio can still see some separation of the files. This does change how things are included, as now the full directory needs to be included instead of only the desired header file. The MSVC filter is unchanged outside adding files which were in-use, but not showing in the MSVC filters (specifically NoteDateStructures.h and OptionsBinding.h).

  2. Removed unused SM5 files. Included files can be seen in commit 08adaec. These files had zero references anywhere else in the source, or were unused altogether.

  3. libtomcrypt and libtommath were moved from the src to the extern/libtomXXX directories. Having them next to in the source of Etterna was out of place as every other library was placed in extern. The way this library gets imported is now done with just the header.

I was able to get it building on both Ubuntu and Windows, though I am currently unable to test myself with macOS.

Please let me know if there is anything I could do to help get this pull request merged.

@jameskr97
Copy link
Member Author

Note: While this commit states that source files have been removed, that is not true. All files have been kept, and only moved. Git will only detect a file as renamed when the source stays exactly the same before and after the change. This is not the case since include references were changed.

@martensm
Copy link
Contributor

Will work on this getting this merged after the 0.65.1 hotfix is released.

@jameskr97
Copy link
Member Author

The last merge should prep this PR for an easy merge into develop. The stb related files were left at their locations in src/ since they'll be getting moved once cmake gets rewritten.

@nico-abram
Copy link
Member

👍

@martensm
Copy link
Contributor

martensm commented Mar 11, 2019

.editorconfig shouldn't be removed, please revert that change: https://editorconfig.org/

@martensm
Copy link
Contributor

martensm commented Mar 11, 2019

I think this PR should be split into 3 separate PRs so it's easier to review, one for each bullet point in the initial comment.

@jameskr97
Copy link
Member Author

@martensm

.editorconfig shouldn't be removed, please revert that change: https://editorconfig.org/

I can do that. Though I just want to confirm it, since this project already has a .clang-format file, which seems to be more recently updated compared to the .editor-format file. The .editor-config file was originally added by the SM5 devs and has remained unmodified since 8e7b4ee7 in 2015. The file's formatting setup does not match the setup used by .clang-format. Are you sure it would be useful to bring the file back?

I think this PR should be split into 3 separate PRs so it's easier to review, one for each bullet point in the initial comment.

I can definitely keep this in mind for next time and make smaller PR's. I understand there a lot of changes happening all at once here. I tried my best to keep each change within its own commit. e45215b to b59ea0f all relate to the first, and while the third is a separate bullet point it's still just moving files. And the second bullet point is all within one commit 08adaec

@nico-abram
Copy link
Member

After some discussiong it seems editorconfig has very few settings, so keeping it consistent with clang format in the future should be trivial. And using it gives the advantage of your editor using the correct tab width automatically without having to run the formatter (On commit or file save). I think the only change needed on the editorconfig file is changing the tab width to 4

@jameskr97
Copy link
Member Author

Sure, I'll push a commit to add it back. I was under the impression it wasn't used whatsoever.

@nico-abram
Copy link
Member

Sure, I'll push a commit to add it back. I was under the impression it wasn't used whatsoever.

Me too

@martensm
Copy link
Contributor

This PR LGTM, I suppose it's not worth splitting it for now.

@nico-abram nico-abram merged commit 7be2863 into etternagame:develop Mar 12, 2019
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.

3 participants