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

NETSCRIPT: placeOrder parameter info changed to reflect BitBurner Docs #1560

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

sahilg2000
Copy link

@sahilg2000 sahilg2000 commented Aug 9, 2024

Linked issues

fixes #74 - TIX: Some documentation doesn't provide guidance on what the input should be

Updated placeOrder parameter docs to show valid inputs as shown on the BitBurner Docs website.

Documentation

Got a few error messages with Netscript but I've only made changes at placeOrder function.

Screenshot (491)

Any suggestions here would be great.

Fixed formatting and linting issues with npm run format and npm run lint.

Fixed code style issues with 'npm run format'.
No linting errors found with 'npm run lint'.
@catloversg
Copy link
Contributor

@d0sboots I'll fix the errors and warnings shown by api-extractor. They are unrelated to this PR.

@catloversg
Copy link
Contributor

@sahilg2000 You should do these things:

  • Add NodeJS to your PATH. line 4: node: command not found means that NodeJS binary (node.exe) is not on your PATH.
  • Check your Git client to see if there are uncommitted files in "markdown" folder. When you change "NetscriptDefinitions.d.ts", there will be changes in that folder.

Comment on lines +1440 to +1447
* @param type - Type of order. Valid values are:
* - "Limit Buy"
* - "Limit Sell"
* - "Stop Buy"
* - "Stop Sell"
* @param pos - Specifies whether the order is a “Long” or “Short” position. Valid values are:
* - "Long"
* - "Short"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, the .md list syntax you're using gets messed up on the sub-pages that get generated by npm run doc. So we can't use this as-is. When I looked into it further, it turns out that the actual code that consumes the values is pretty lenient, so I suggest this:

Suggested change
* @param type - Type of order. Valid values are:
* - "Limit Buy"
* - "Limit Sell"
* - "Stop Buy"
* - "Stop Sell"
* @param pos - Specifies whether the order is a “Long” or “Short” position. Valid values are:
* - "Long"
* - "Short"
* @param type - Type of order. Must contain one of "limit" or "stop", and one of "buy" or "sell".
* @param pos - Specifies whether the order is a “Long” or “Short” position. Must contain the letter "l" or "s".

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use this PR as an opportunity to explicitly tell players the "expected" value? I really don't like the fuzzy matching of our old APIs. If we ever change the implementation (in a major version), it's best to tell them what we expect now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose, except I'm not sure what exactly the expected value would be. The enum value is about the only thing that truly makes sense, but I don't think people will write out "Limit Buy Order".

Ultimately, I think we have to accept that the fuzzy matching on this one won't change.

Copy link
Contributor

@catloversg catloversg Aug 15, 2024

Choose a reason for hiding this comment

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

Using enum is a good idea.

declare enum StockOrderType {
  LimitBuy = "LimitBuy",
  LimitSell = "LimitSell",
  StopBuy = "StopBuy",
  StopSell = "StopSell",
}

declare enum StockPosition {
  Long = "Long",
  Short = "Short",
}

placeOrder(
  sym: string,
  shares: number,
  price: number,
  type: StockOrderType | `${StockOrderType}`,
  pos: StockPosition | `${StockPosition}`,
): boolean;

I think this is fine. It does not break the backward compatibility, clearly tell players which value they should use, and the autocomplete feature can help them.

Copy link
Collaborator

@d0sboots d0sboots Aug 15, 2024

Choose a reason for hiding this comment

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

We already have the enums PositionType and OrderType. I would use those rather than creating new, slightly different versions.

I'm OK with lying slightly in the type in order to improve autocomplete and make a better dev experience, but I think I'd want to keep the comment the way I worded it in that case. It's very confusing when functions don't work the way they are specified; even if the way they work is not what we'd prefer, I think it's more important that the docs be correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have the enums PositionType and OrderType. I would use those rather than creating new, slightly different versions.

You are right. I did not see those two when I wrote the example.

I'm OK with lying slightly in the type in order to improve autocomplete and make a better dev experience, but I think I'd want to keep the comment the way I worded it in that case. It's very confusing when functions don't work the way they are specified; even if the way they work is not what we'd prefer, I think it's more important that the docs be correct.

I understand.

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

Successfully merging this pull request may close these issues.

TIX: Some documentation doesn't provide guidance on what the input should be
3 participants