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

discuss: List syntax for docking ports. #12

Closed
frencrs opened this issue Mar 21, 2014 · 13 comments
Closed

discuss: List syntax for docking ports. #12

frencrs opened this issue Mar 21, 2014 · 13 comments

Comments

@frencrs
Copy link

frencrs commented Mar 21, 2014

The previous version of kOS used:

list DOCKINGPORTS from VESSEL in x.

The new syntax:

LIST (IDENTIFIER (IN IDENTIFIER)?)? EOI;

Any ideas on what the best method for a dockingport command would be?

@frencrs
Copy link
Author

frencrs commented Mar 21, 2014

Simply putting this in Suffixed.VesselTarget:

public ListValue GetDockingPorts()
{
      return DockingPortValue.PartsToList(Vessel.Parts);
}

and adding the suffix "DOCKINGPORTS" to call it, seems to work.

Users can use:

set d to TARGET:DOCKINGPORTS.

to get a list object in d that dockingports can be selected from to target.

@erendrake
Copy link
Member

I am hesitant to add yet another property to VesselTarget, It is getting unwieldy

@marianoapp
Copy link
Contributor

I completely missed the LIST ... FROM ... IN ... syntax when I merged the code.
We'll have to add support for this changing the LIST syntax to "LIST (IDENTIFIER ((FROM IDENTIFIER)? IN IDENTIFIER)?)? EOI;" and updating the compiler to work with the new syntax.

@erendrake
Copy link
Member

@marianoapp I noticed we lost that last night and I was going to ask you about it :) About adding a vessel property I would feel more comfortable adding TARGET:PART:(ENGINE|DOCKINGPORT|TANK|ECT|...)

@xyztdev
Copy link
Contributor

xyztdev commented Apr 11, 2014

I personally dislike the list .. in syntax and think that all lists should be declared in this way:

set d to TARGET:DOCKINGPORTS.

@erendrake
Copy link
Member

@xyztdev again, I do not want to add a ton of new properties to Vessels that your suggestion would require. adding a single property for "part" would be acceptable but i dont think we are far enough along to have multiple ways to do things yet.

@Dunbaratu
Copy link
Member

I guess one advantage that listing the parts into a user list has is that it very clearly enforces a read-only style of thinking on the part of the script programmer, and makes it clear that getting the list is an expensive operation rather than something you'd want to repeat over and over. Making it look like part of the structure makes you think you can add/remove things from the vessel, or that it's no big deal to keep asking for TARGET:DOCKINGPORTS again and again in a loop. Having to LIST the docking ports into your own variable makes it crystal clear that, (A) editing the list won't affect anything except your own copy of the list, and (B) It's not a good idea to keep asking for the same query again when you already have the data in your own list.

@xyztdev
Copy link
Contributor

xyztdev commented Apr 13, 2014

@Dunbaratu This is not consistent with other properties. APOAPSIS cannot be set. ENCOUNTER cannot be set. Many other properties cannot be set.

@xyztdev
Copy link
Contributor

xyztdev commented Apr 13, 2014

@erendrake I would support the part suggestion.

@Dunbaratu
Copy link
Member

I concede your point about there being other read-only properties, but that's not the most important part of my comment. The most important part is that it drills home the point that you shouldn't just keep asking kOS to re-derive the list again and again because that's extra work.

If it was a property then people might think it was okay to do this:

set pNum to 0.
until pNum >= SHIP:PARTS:LENGTH {
  print "Part number " + pNum + " is " + SHIP:PARTS#pNum.
  set pNum to pNum + 1.
}.

And not realize that every time they say SHIP:PARTS they're making kOS assemble the the ship part list again. Making it something you have to store in your own variable first makes it clear that it's something you only want to do once up front.

@erendrake
Copy link
Member

@Dunbaratu is correct. That is why i did it this way. there are certainly other ways that will make sense but i think for now having one way that works will suffice

@Dunbaratu
Copy link
Member

To make it work as a suffix property without the expense of re-generating the list each time, you'd have to retain an internal representation of the list, but implement it with an internal "dirty" flag that tracks whether or not something happened that could change the parts list, and then if it's not "dirty" then it doesn't re-build the list. It's doable, but more work and opens up the chance for error if not done very carefully.

@xyztdev
Copy link
Contributor

xyztdev commented Apr 14, 2014

It's not the hardest problem to solve; it's similar to web catching in a way. I might submit something if I have the time.

@erendrake erendrake added this to the v12.2 Adding features at last. milestone May 10, 2014
erendrake pushed a commit that referenced this issue Apr 14, 2015
Updating to latest version of develop
erendrake pushed a commit that referenced this issue May 13, 2015
Pulling changes from the main repo
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

No branches or pull requests

5 participants