Autodesk-AutoCAD / AutoLispExt

Visual Studio Code Extension for AutoCAD® AutoLISP
https://marketplace.visualstudio.com/items?itemName=Autodesk.autolispext
Apache License 2.0
83 stars 30 forks source link

Documentation updates related to acet-* functions #228

Closed ambrosl-adsk closed 1 year ago

ambrosl-adsk commented 1 year ago
Objective

Provide access to the online documentation for the acet-* functions that are documented.

Abstractions
Tests performed

Added all of the documented acet-* functions to an LSP file and made sure each opened the correct online help topic with the "Open Online Help" command. I also checked the syntax that comes up in the tooltip when hovering over each function.

Screen shot

image image image

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot: Thank you for your submission, we really appreciate it. We ask that you sign our Contributor License Agreement before we can accept your contribution.

-->If you are contributing on behalf of your employer: you must fill out our Corporate Contributor License Agreement which can be found here, and send the signed forms to autolispext.contributor.agreements@autodesk.com.

-->If you are contributing on behalf of yourself: you must agree to our Individual Contributor License Agreement which can be found here, and either send the signed forms to autolispext.contributor.agreements@autodesk.com, or reply below with a comment containing the following text:


I have read the CLA Document and I hereby sign the CLA


1 out of 2 committers have signed the CLA.
:white_check_mark: @ambrosl-adsk
:x: @Sen Lin Application Admin
Sen Lin Application Admin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

JD-Howard commented 1 year ago

@ambrosl-adsk Could you please replace the webHelpAbstraction.json instead of updating the zip file.

The zip really exists as an artifact from the C# processing. Since this is all pencil whipped, there really isn't a reason to update the zip.

ambrosl-adsk commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

ambrosl-adsk commented 1 year ago

@ambrosl-adsk Could you please replace the webHelpAbstraction.json instead of updating the zip file.

The zip really exists as an artifact from the C# processing. Since this is all pencil whipped, there really isn't a reason to update the zip.

@JD-Howard Sen pointed that out to me... and was updated separately in a new commit to this PR.

JD-Howard commented 1 year ago

acet-reg-prodkey exists in the docs but not our highlighting syntax. We just need to add it to line 163 on \extension\syntaxes\autolisp.tmLanguage.json

I checked all the guid links but I didn't audit any of the actual documentation. I saw random things in there (missing ?) that isn't congruent but its also not important.

Generally this looks pretty good and more importantly functional. One thing you should probably consider doing though...

Move acet-ui-progress into the ambiguousFunctions

Basically that function has multiple distinctly different signatures and thus really belongs in ambiguousFunctions. I also noticed that the official docs do not directly (in signatures) list the perfectly valid no-args arrangement.

Edit: also if you want to keep the official docs consistent, you should remove the "and" from that acet-ui-progress page. here is an example of an existing ambiguous functions documentation: https://help.autodesk.com/view/OARX/2024/ENU/?guid=GUID-4C3B6017-BA57-451D-8CF8-C8539E9517F6

ambrosl-adsk commented 1 year ago

@JD-Howard acet-reg-prodkey is no longer supported, I did include it in the documentation/syntax as a way to indicate it is Obsolete. So the short description does start with (Obsolete).

I wasn't exactly sure about the ?, some of the existing had it in places under syntax but not the argument. Assuming its intent is to indicate optional arguments. If the ? should follow the argument name in the JSON I can add it to be consistent.

acet-ui-progress without any arguments is really the syntax of (acet-ui-progress [label [max]]). I can move it to the ambiguousFunctions where vlax-3D-point is located and parallel its signature.

JD-Howard commented 1 year ago

I wasn't exactly sure about the ?, some of the existing had it in places under syntax but not the argument. Assuming its intent is to indicate optional arguments. If the ? should follow the argument name in the JSON I can add it to be consistent.

You generally got the ? correct in the signatures, they just often didn't exist in the arguments sections. Which is fine and not a big deal. The other thing I did notice is that the documentation had "named" versions for enums + the integer value. If the names exist (IE, are symbols that resolve to integers) then we should just use the names. Again, at this point I don't consider that much of an issue. The parser (when updated) can pluck those out later.

Sen-real commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

JD-Howard commented 1 year ago

@ambrosl-adsk I consider this temporary and I don't care either way, but Sen requested my review and I am just waiting to figure out if that 1 function is getting migrated into the ambiguous area.

At minimum, acet-reg-prodkey may be deprecated, but it still valid for some environments and thus needs to be syntax highlighted.

Sen-real commented 1 year ago

At minimum, acet-reg-prodkey may be deprecated, but it still valid for some environments and thus needs to be syntax highlighted.

Yes I agree, as it already works well with online help, it should also be highlighted. I'll add it into the highlighting config file. image