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

File 'content_sao' splitted into folder 'src/server/object'. #7903

Closed
wants to merge 2 commits into from

Conversation

Unarelith
Copy link
Contributor

So as @SmallJoker asked, I'm splitting #7900 into two distinct PRs.

@rubenwardy
Copy link
Member

rubenwardy commented Nov 26, 2018

PR lgtm, but I haven't done a diff to check for modifications

To do:

  • Update Android makefile source list

- [ ] Get approval for moving to .hpp

@rubenwardy rubenwardy added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements @ Server / Client / Env. and removed @ Server / Client / Env. labels Nov 26, 2018
Copy link
Member

@nerzhul nerzhul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

header files should be .h instead of .hpp

@paramat
Copy link
Contributor

paramat commented Nov 26, 2018

Thanks.
I like what this does, but if it conflicts with any of the blocker PRs i think those have priority for merge https://github.com/minetest/minetest/pulls?q=is%3Aopen+is%3Apr+label%3ABlocker

@paramat
Copy link
Contributor

paramat commented Nov 27, 2018

This PR seems good to me in concept.

@paramat
Copy link
Contributor

paramat commented Nov 29, 2018

Concerning filenames, in #7902 moved files are not renamed, i guess there is some advantage to this. But where you are creating new files you are using PascalCase with no underscores. I think underscores make filenames more readable.
It's also the project convention and a dev rule:
https://dev.minetest.net/Code_style_guidelines
"Files should be named using lowercase_underscore style."
Since we are going to have many more files with your changes it's even more important that the filenames are very readable.

@paramat
Copy link
Contributor

paramat commented Nov 29, 2018

More thoughts on filenames, see #7908 (comment) onwards. Removed my -1.

@paramat
Copy link
Contributor

paramat commented Dec 3, 2018

As decided elsewhere, snake_case please.

@paramat
Copy link
Contributor

paramat commented Apr 3, 2019

@nerzhul 's request "header files should be .h instead of .hpp" seems done.

@paramat
Copy link
Contributor

paramat commented Apr 4, 2019

I prefer .h too:

  • Less work.

  • mapgen.cpp looks visually quite similar to mapgen.hpp, only one character differs, so it is visually easier to distinguish files when using .h.

@paramat paramat added the Rebase needed The PR needs to be rebased by its author. label Apr 4, 2019
@rubenwardy rubenwardy force-pushed the master branch 2 times, most recently from 0e3b135 to 39c54e1 Compare June 21, 2019 00:46
@Unarelith
Copy link
Contributor Author

Hey @paramat, I forgot this one. The goal of this PR was also to split a big file into multiple smaller files.

@nerzhul
Copy link
Member

nerzhul commented Apr 17, 2020

i missed that PR, but now it's useless, i did the refacto + some code cleanups myself last week

@nerzhul nerzhul closed this Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Rebase needed The PR needs to be rebased by its author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants