Page MenuHomeWildfire Games

gather technology naming consistency
Needs ReviewPublic

Authored by Nescio on Sep 18 2017, 1:12 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

This patch renames gather technologies to be more consistent and precise, e.g.:

gather_metal_city.json
gather_metal_town.json
gather_metal_village.json
gather_stone_city.json
gather_stone_town.json
gather_stone_village.json

The advantage is that from the file name alone it's immediately clear what they do and what their order is. Moreover, searching for e.g. city will show up technologies enabled in city phase and metal will show up all metal technologies.

This is currently not the case, e.g.:

gather_mining_serfs.json
gather_mining_servants.json
gather_mining_shaftmining.json
gather_mining_silvermining.json
gather_mining_slaves.json
gather_mining_wedgemallet.json

You have to open them to find out which one you really need, which is quite inefficient.

gather_animals_stockbreeding.json
gather_capacity_fishing.json
gather_fishing_net.json

These technologies affect domestic animals and fishing boats, respectively, not citizens/gatherers/workers, therefore they are renamed to reflect that.

Test Plan

Verify everything works as before.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3211
Build 5558: Vulcan BuildJenkins
Build 5557: arc lint + arc unit

Event Timeline

Nescio created this revision.Sep 18 2017, 1:12 PM
Owners added a subscriber: Restricted Owners Package.Sep 18 2017, 1:12 PM
Vulcan added a subscriber: Vulcan.Sep 18 2017, 2:32 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2051/ for more details.

Nescio updated this revision to Diff 3772.Sep 25 2017, 10:49 AM
Nescio edited the summary of this revision. (Show Details)

Updated (necessary because of minor file conflict with rP20210; solved)

@Nescio:
Namming in that folder is indeed not always consistent (due to different people having different (or no) policies).
Just to grab your input. (A patch with things to rename, to move, to delete, to add, doesn't really attract people if there isn't an obvious benefit)

There are 2 things in the patch:

  • making it more explicit
  • removing name from the tech and replacing it with a number

I won't discuss the 1st point as I have no opinion about that.
For the 2nd point, what happens if we want to change the order or include other same kind of tech?

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2068/ for more details.

Admittedly, the benefit of this patch (and of D888) is rather minor. However, the advantage of naming consistency is that it's easier to edit, expand, maintain, and modify. It doesn't limit possibilities, it might be helpful, therefore it's useful to implement, is my reasoning.

As for your last question, if you want to change the order of technologies, the two easiest options are:

  • keep the file names and supersedes tags unchanged and change the description etc.
  • keep the description etc. unchanged and swap the file names and supersedes tags

Basically this is the same you would have to do in the current situation. Renaming files doesn't change anything fundamental.

Nescio added a reviewer: Restricted Owners Package.Dec 23 2019, 4:25 PM
Nescio updated this revision to Diff 12524.Jul 2 2020, 5:17 PM
Nescio retitled this revision from technology naming consistency: storehouse and farmstead researches to gather technology naming consistency.
Nescio edited the summary of this revision. (Show Details)
Nescio removed a subscriber: Restricted Owners Package.
  • redone from scratch
Owners added a subscriber: Restricted Owners Package.Jul 2 2020, 5:17 PM
Nescio added a subscriber: wraitii.Jul 2 2020, 5:18 PM

@wraitii (or another team member), are you willing to review this?

Vulcan added a comment.Jul 2 2020, 5:22 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2567/display/redirect

Nescio edited the test plan for this revision. (Show Details)Jul 12 2020, 9:58 AM

@Freagarach, are you willing to review this?

Actually, I like the technologies being named after their name more. Therefore I won't commit this, unless there more people who think this is a change for the better, in which case I may be willing to put my own preference aside.

I feel like something like this (order random)

gather_metal_0_serfs.json
gather_metal_1_servants.json
gather_metal_2_shaftmining.json
gather_stone_0_silvermining.json
gather_stone_1_slaves.json
gather_stone_2_wedgemallet.json

might be reasonably efficient at conveying order, but also nicer than hardcoding "city"

The thing is, you'll have to move files around when changing the order, which is not really nice, IMHO.

The thing is, you'll have to move files around when changing the order, which is not really nice, IMHO.

I mean you would need to change the names anyway, no?

Nescio added a comment.Aug 6 2020, 9:21 AM

Actually, I like the technologies being named after their name more.

For comparison, many (though not all) technologies currently have ordered file names without including their generic (displayed) name, e.g.:

armor_cav_01.json
armor_cav_02.json
attack_infantry_ranged_01.json
attack_infantry_ranged_02.json
heal_range.json
heal_range_2.json
pop_house_01.json
pop_house_02.json
speed_cavalry_01.json
speed_cavalry_02.json
trade_gain_01.json
trade_gain_02.json

This makes it immediately clear which one you need.
As to why I opted for _village, _town, and _city in this patch instead, is because there is one gather technology of every type for each phase; it's highly unlikely their phase requirements will ever be changed.

The thing is, you'll have to move files around when changing the order, which is not really nice, IMHO.

I mean you would need to change the names anyway, no?

Moreover, if the generic name (which is just a string) of a technology is changed, then the file would have to be moved too. This is no longer necessary with this proposal.