Modify scene references to reduce encapsulation

akien-mga / dynadungeons

Bomberman clone using Godot Engine - Not actively developed since 2015.

GNU General Public License v3.0

216 stars 36 forks source link

Modify scene references to reduce encapsulation #22

Closed ghost closed 9 years ago

ghost commented 9 years ago

As discussed in previous PR :)

akien-mga commented 9 years ago

Looks good :) Two comments:

Could you try to use git pull --rebase to update your fork? It should get rid of useless commits like 542ed3e. Edit: You can still do it now, and maybe force push the update if need be, it will also update this PR.
If you look at the diff on GitHub for src/player.gd, there seems to still be a "No newline at end of file" issue even though you added a newline. I guess it might come from the settings of your text editor, I'll see if there's a way to enforce final newlines using .gitattributes.

akien-mga commented 9 years ago

Some context about the newline stuff: https://robots.thoughtbot.com/no-newline-at-end-of-file I'll see if forcing LF line endings will help for those EOF issues. Out of curiosity, which editor are you using and on which platform?

ghost commented 9 years ago

Sorry about that. I got about confused about how to update the fork. I've run rebase now but I'm not sure if it did anything. Should I be getting rid of that commit and then running rebase?
Yeah, I'm not sure what's up, since I used godot's script editor on Ubuntu 14.04. Maybe if I try using something like Atom?

akien-mga commented 9 years ago

Sorry about that. I got about confused about how to update the fork. I've run rebase now but I'm not sure if it did anything. Should I be getting rid of that commit and then running rebase?

Check the git log, normally the merge commit should have been removed.

Yeah, I'm not sure what's up, since I used godot's script editor on Ubuntu 14.04. Maybe if I try using something like Atom?

That's weird, I'm also using the Godot editor on Mageia 5 but don't have this kind of issues. I'll experiment a bit, maybe it would be worth a bug report upstream.

ghost commented 9 years ago

Hmmm, was still there in the log. I'm a bit over the place but I've reverted to before the commits and run rebase, which seems to have sorted things. I'll redo the useful commit and re-open the PR.

ghost commented 9 years ago

Will also try using a different text editor to insert the newline

ghost commented 9 years ago

Okay, I think we're good now, lesson learned :)

I think using Atom got me around the newline problem. Perhaps the Godot editor isn't equipped to deal with newline characters, but leaves existing ones alone?

akien-mga commented 9 years ago

Not really good, check the diff before committing ;)

Atom removed all leading tabulations on empty lines, so the commit is pretty hard to read. Usually I would agree that empty lines should not have tabulations, but due to the way the Godot editor enforces tabulations, I think it's better to keep them to properly mark where blocs start and end.

If the Godot editor was more clever, I'd gladly do indentation with spaces instead of tabulations, and remove spaces on cosmetic empty lines, but sadly it's not easily done right now :)

akien-mga commented 9 years ago

Sorry about nit-picking btw. I guess I'm a bit maniac, clear commits and pull requests are almost as important to me as the feature they implement :P

ghost commented 9 years ago

Haha yeah, all good, better to do it right first time :)

Darn. I'll roll back the commit then, add the newline using Atom, fix the tabulations in Godot, check the diff, and then commit.

ghost commented 9 years ago

Fingers crossed that this is the last one!

akien-mga commented 9 years ago

Yeah it's just fine now :-)

Actually about the newline thing, you added a new line (empty line 219) when it should have been only an invisible "newline" character at the end of lien 218. This character is badly named, it's more of a line break or an "end of line" character. The link I gave above gives more details about where this comes from historically, etc.

I'll investigate a bit more when I'm home, but this PR is already perfect :)

ghost commented 9 years ago

Interesting. Do you mean an escape character like '\n'?

Though I suppose that would still be visible. On 25/08/2015 9:42 pm, "Rémi Verschelde" notifications@github.com wrote:

Merged #22 https://github.com/akien-mga/dynadungeons/pull/22.

— Reply to this email directly or view it on GitHub https://github.com/akien-mga/dynadungeons/pull/22#event-391497944.

akien-mga commented 9 years ago

Interesting. Do you mean an escape character like '\n'?

Yep, so basically the script should end with:

set_fixed_process(true)\n

while right now, IINM (based only on what GitHub shows) it ends with:

set_fixed_process(true)\n
\n

ghost commented 9 years ago

Cool. Will give that a shot then :)