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

docs: extract out common types #11391

Merged
merged 42 commits into from Dec 17, 2019
Merged

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Dec 12, 2019

JIRA
https://jira.appcelerator.org/browse/TIMOB-27688

Description:

  • Updates to titanium-docgen 4.1.0 which now warns about using generic Dictionary/Object types
  • Extracted out Size type: width and height
  • Extracted Dimension type from View's yml file, have it extend Size and add x and y properties
    • Ti.Blob.#imagesAsCropped() now refers to Dimension rather than a custom type that had the same properties.
    • Ti.Media CameraMediaItemType now refers to Dimension for cropRect and Size for previewRect rather the defining custom types with same properties.
  • Extracted Padding type from ViewPadding. Attempted to clean up any special types that had the same concept (basically any padding/inset properties/types) to refer to it.
    • Kept TabIconInsets, had it extend Padding and noted right/bottom are excluded.
    • Kept TextFieldPadding, had it extend Padding and noted top/bottom are Android-only.
  • Fix Ti.DB.ResultSet #field() and #fieldByName() returns to be a single entry with type as an array of possible return types.

@sgtcoolguy
Copy link
Contributor Author

cc @drauggres You might be interested in these changes

@build build added this to the 9.0.0 milestone Dec 12, 2019
@build build requested a review from a team December 12, 2019 16:07
@build
Copy link
Contributor

build commented Dec 12, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 700 skipped out of 7381 total tests.

Tests:

ClassnameNameTimeError
ios.ipad.Titanium.Blob#toString() returns "[object TiBlob] for binary content (13.2.2)0.043
Error: expected undefined to equal '[object TiBlob]'
fail@file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/node_modules/should/cjs/should.js:275:19
value@file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/node_modules/should/cjs/should.js:356:13
file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti.blob.test.js:386:28
file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:54167
file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:58607
file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:59452
next@file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:58015
file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:57979
next@file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:57667
file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:6535:57763
timeslice@file:///Users/build/Library/Developer/CoreSimulator/Devices/E6250863-45A7-40F1-9840-23A71FEA8F02/data/Containers/Bundle/Application/C62950F9-13B8-4BFF-857C-544ED8C4475D/mocha.app/ti-mocha.js:5764:23

Dependencies with modified semantic versioning:

Generated by 🚫 dangerJS against f300479

@sgtcoolguy sgtcoolguy marked this pull request as ready for review December 16, 2019 15:38
@sgtcoolguy
Copy link
Contributor Author

Relates to #11386

@janvennemann Perhaps you'd be best for review? I attempted to try and basically "refactor" our docs to use some commonly defined types for many of the "simple object which has a few properties" arguments/callbacks. The goal was to clean the docs, try and re-use common types when defining them, and hopefully to improve TypeScript definitions and IDE content assist indirectly.

type: zoomScaleOption
- name: options
summary: A JS object with an `animated` property which determines whether the scrollable region reposition should be animated
type: AnimatedOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

zoomScaleOption was iOS-only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true, but this method is iOS-only so the method and any arguments are therefore iOS-only.

summary: Determines whether the scrollable region reposition should be animated
type: contentOffsetOption
summary: A JS object with an `animated` property which determines whether the scrollable region reposition should be animated
type: AnimatedOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

contentOffsetOption was iOS-only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's true, but this method is iOS-only so the method and any arguments are therefore iOS-only.

@@ -1147,7 +1149,7 @@ properties:
description: |
This was separated from the <Titanium.UI.Window.androidback> event. You need to define this
callback if you explicitly want to override the back button behavior.
type: Callback<Object>
type: Callback<void>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct way to define callback with no arguments? I can't find any other Callback<void> (except one in validate.js from titanium-docgen).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I believe it is in our doc format. Callback is synonymous with Function. As to the void arg, well that is seen as ok in the validation. Otherwise, I'm not sure how to define that the function takes no arguments. I think the generators are ok with with void arg

summary: Includes options how the menu popup should be hidden.
type: MenuPopupHideParams
summary: Includes options how the menu popup should be hidden. Introduced in SDK 5.2.0
type: AnimatedOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Default was {animated: true}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I struggled with how to handle the cases where the default animated value is true vs false. Do we just mention it in the summary? Do I define a new sub-type of AnimatedOptions that just overrides the property's default value to be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To do some follow-up: These special "types" I've been extracting are really a better match as TypeScript interfaces, not classes. Because there's no actual way to instantiate an AnimatedOptions type - no such thing exists. It's more about the shape of a generic JS object we expect to be passed in to the methods. As such, the conversion to TypeScript would drop any "default" values on objects like this. It's more about documenting the property names, whether they're optional/required, and if they're "readonly". So trying to set the default value on the type may actually be moot - and I may need to just be thorough in mentioning the assumed defaults on all the methods that accept it. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just thought of another possible way to document this - set the default value on the method argument to the raw JS object that it "assumes". Old docs already do this for things like Point (i.e. { x: 0, y: 0 }) We've been pretty loosey-goosey with how we handle default though. Not sure if we'd need more rules around it or just assume that the string value is the raw JS code/value that is assumed to be default. (How would we validate that, since I know we do have cases where we have sentences there?)

Copy link
Contributor

Choose a reason for hiding this comment

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

At least we should keep this information in summary or description (same for iOS-only properties).

@@ -39,14 +39,14 @@ methods:
parameters:
- name: options
summary: Display properties to use when hiding the popover.
type: PopoverParams
type: AnimatedOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Default was {animated: true}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above


- name: show
summary: Displays the popover.
parameters:
- name: params
summary: Display properties to use when displaying the popover.
type: PopoverParams
type: ShowPopoverParams
Copy link
Contributor

Choose a reason for hiding this comment

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

Default was {animated: true}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

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

Only two minor notes, the rest looks good.

PS: Man this looks like some tedious work, awesome job!

apidoc/Titanium/UI/Padding.yml Outdated Show resolved Hide resolved
apidoc/Titanium/UI/View.yml Outdated Show resolved Hide resolved
optional: true

- name: UIApplicationOpenURLOptionsAnnotationKey
type: Array
Copy link
Contributor

Choose a reason for hiding this comment

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

Here must be a type of items stored in the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I couldn't find what the heck goes in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: with type: Array current version of the "typescript" generator will produce incorrect output.

@sgtcoolguy sgtcoolguy merged commit 5cdbdf0 into tidev:master Dec 17, 2019
@sgtcoolguy sgtcoolguy deleted the titanium-docgen-4.1.0 branch December 17, 2019 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants