r8883 - Inventory output reversed/altered

Forum closed. All further discussion to be discussed at https://github.com/OpenKore/

Moderators: Moderators, Developers

Message
Author
User avatar
ChrstphrR
Testers Team
Testers Team
Posts: 42
Joined: 09 May 2010, 17:30
Noob?: No
Location: Northern Alberta, Canada

r8883 - Inventory output reversed/altered

#1 Post by ChrstphrR »

Not that I mind the change, rather, it's just not DOCUMENTED as a change, in amongst the hundreds of lines altered in this commit in Commands.pm

"Old" r8882 and earlier inventory looks as follows:

Code: Select all

-----------Inventory-----------
-- Equipment (Equipped) --
6    Novice Adventurer's Suit [Pupa] [1] -- Armor (16)
7    Clip [1] -- Right Accessory (8)
9    +7 Muffler [1] -- Cape (4)
10   +4 Hunter Bow -- Two-Handed Weapon (34)
-- Equipment (Not Equipped) --
5    Steel Arrow (Arrow) x 50
-- Non-Usable --
2    Eden Group Mark x 1
-- Usable --
0    Novice Fly Wing x 42
1    Novice Butterfly Wing x 24
3    Yellow Butterfly Wing x 1
-------------------------------
"New" r8883 inventory output:

Code: Select all

------------------- Inventory --------------------
-- Usable --
0    Novice Fly Wing x 42
1    Novice Butterfly Wing x 24
3    Yellow Butterfly Wing x 1

-- Equipment (Not Equipped) --
5    Steel Arrow (Arrow) x 50

-- Non-Usable --
2    Eden Group Mark x 1

-- Equipment (Equipped) --
6    Novice Adventurer's Suit [Pupa] [1] -- Armor (16)
7    Clip [1] -- Right Accessory (8)
9    +7 Muffler [1] -- Cape (4)
10   +4 Hunter Bow -- Two-Handed Weapon (34)
--------------------------------------------------
Dare I ask how many other commands I haven't looked at thoroughly were altered more than just a "translation" message update in this module?

Was this changeset tested at all before being committed?

<snotty opinion somewhat grounded in experience>
There's a reason why we write commit messages, and limit the commit to what we say has been changed.

When an author commits a change -- one change, that is -- it reduces the complexity to test that one change by several orders of magnitude when the one feature changed is isolated, instead of leaving the change to be fixed.

When dozens (hundreds?) of changes are made, and combined together in one massive update, this makes bug hunting post-commit much more involved, and pre-commit, it leaves the coder/tester many more opportunities to miss checking and testing for unintended changes their massive update can cause.

IMO, this commit should've been at least 4, given the four unrelated notes in the commit message.
However, the translation updates to Commands.pm should have been several dozen commits all by themselves.
</snotty opinion somewhat grounded in experience>

User avatar
4epT
Developers
Developers
Posts: 617
Joined: 30 Apr 2008, 14:17
Noob?: No
Location: Moskow (Russia)
Contact:

Re: r8883 - Inventory output reversed/altered

#2 Post by 4epT »

Yes, I tested. Do not worry.

I replaced all of the expressions of the form:

Code: Select all

message T("------ AI Sequence ---------------------\n"), "list";
on:

Code: Select all

my $msg = center(T(" AI Sequence "), 50, '-') ."\n";
All my posts are made by machine translator!
¤ Manual ¤ Anti BotKiller ¤ Packet Extractor v3 ¤
Image
Image

User avatar
itsrachelfish
Developers
Developers
Posts: 50
Joined: 27 Feb 2012, 12:50
Noob?: No

Re: r8883 - Inventory output reversed/altered

#3 Post by itsrachelfish »

If it's true that all you did was replace the formatting for those lines, why would the order of the information change?
Tired of waiting for answers on the forums?
Want to talk to OpenKore experts?

Hang out on the #OpenKore IRC!

User avatar
ChrstphrR
Testers Team
Testers Team
Posts: 42
Joined: 09 May 2010, 17:30
Noob?: No
Location: Northern Alberta, Canada

Re: r8883 - Inventory output reversed/altered

#4 Post by ChrstphrR »

4epT wrote:Yes, I tested. Do not worry.
Please explain how a search and replace code change on the msg() routine altered Inventory ("i" in Commands.pm).

Quite obviously MORE than just the lines using msg() were affected, or this side effect wouldn't occur.

Found another bug in the interim, which crashes kore, details to follow shortly, in another post since it's another issue.

User avatar
4epT
Developers
Developers
Posts: 617
Joined: 30 Apr 2008, 14:17
Noob?: No
Location: Moskow (Russia)
Contact:

Re: r8883 - Inventory output reversed/altered

#5 Post by 4epT »

ChrstphrR wrote:Please explain how a search and replace code change on the msg() routine altered Inventory ("i" in Commands.pm).
what for?

replace the function "sub cmdInventory"
All my posts are made by machine translator!
¤ Manual ¤ Anti BotKiller ¤ Packet Extractor v3 ¤
Image
Image

User avatar
ChrstphrR
Testers Team
Testers Team
Posts: 42
Joined: 09 May 2010, 17:30
Noob?: No
Location: Northern Alberta, Canada

Re: r8883 - Inventory output reversed/altered

#6 Post by ChrstphrR »

4epT wrote:
ChrstphrR wrote:Please explain how a search and replace code change on the msg() routine altered Inventory ("i" in Commands.pm).
what for?

replace the function "sub cmdInventory"
From your commit comment:

Code: Select all

- BIG update message translation
- updated "disconnection" options
- update ServerType0, Sakexe_0, Sakexe_2005_05_30a, Sakexe_2005_06_22a, RagexeRE_2013_03_20
- add new sT RagexeRE_2013_05_15a
So, where is "changed cmdInventory" in that comment? How many other undocumented changes and side effects are in this massive commit?

User avatar
4epT
Developers
Developers
Posts: 617
Joined: 30 Apr 2008, 14:17
Noob?: No
Location: Moskow (Russia)
Contact:

Re: r8883 - Inventory output reversed/altered

#7 Post by 4epT »

I changed only way to display messages.
All my posts are made by machine translator!
¤ Manual ¤ Anti BotKiller ¤ Packet Extractor v3 ¤
Image
Image

User avatar
ChrstphrR
Testers Team
Testers Team
Posts: 42
Joined: 09 May 2010, 17:30
Noob?: No
Location: Northern Alberta, Canada

Re: r8883 - Inventory output reversed/altered

#8 Post by ChrstphrR »

Please fix the issue stated.

Otherwise, there are now at least two outstanding bugs I found, and the only choice left is to revert the code to 8882.

---

Пожалуйста, решить проблему заявил.

В противном случае, в настоящее время по крайней мере, два выдающихся ошибки, которые я нашел, и единственным выбором остается только вернуться код для 8882.

User avatar
4epT
Developers
Developers
Posts: 617
Joined: 30 Apr 2008, 14:17
Noob?: No
Location: Moskow (Russia)
Contact:

Re: r8883 - Inventory output reversed/altered

#9 Post by 4epT »

All my posts are made by machine translator!
¤ Manual ¤ Anti BotKiller ¤ Packet Extractor v3 ¤
Image
Image

User avatar
ChrstphrR
Testers Team
Testers Team
Posts: 42
Joined: 09 May 2010, 17:30
Noob?: No
Location: Northern Alberta, Canada

Re: r8883 - Inventory output reversed/altered

#10 Post by ChrstphrR »

Commands.pm was not altered in this commit, so there is no possible way this issue was fixed there.

Actually fixed with r8886, r8887:

http://sourceforge.net/p/openkore/code/8886/
http://sourceforge.net/p/openkore/code/8887/

The two were separated out, only because update.sh in /src/po/update.sh would not update the *.po files from my working copy with modifications made to Commands.pm made in 8886.

r8886

Code: Select all

Altered output of cmdInventory back to pre 8883 behavior, that is, item order changed back to eq|neq|nu|u, and extraneous linefeeds removed.

Fixes bug (undocumented side effect) first present in revision 8883 reported at:
 http://forums.openkore.com/viewtopic.php?f=56&t=209406&start=0&sid=4fc89522fd4d5aa6c85f0bafb7aee8ec

- Removed one-liner comment for cmdInventory description
 - And added POD style comment block describing the behaviour of the routine instead
 - Restored order of display of inventory items to eq|neq|nu|u, to conform to description found in the routine's comment header.
 - Updated "Translation Comment" notes for some of the entries in cmdInventory

Translation *.po files will be updated following this commit!
r8887

Code: Select all

Translation file updates to fit with revision 8886, last part of fix to resolve 

bug (undocumented side effect) first present in revision 8883 reported at:
 http://forums.openkore.com/viewtopic.php?f=56&t=209406&start=0&sid=4fc89522fd4d5aa6c85f0bafb7aee8ec

- Translation *.po files have cmdInventory strings updated
 - Translation files updated via excuting src/po/update.sh

Locked