Remove Accessors #12310

sgtcoolguy · 2020-12-02T20:58:46Z

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

Description:
This PR removes the getter/setter methods for properties where we can directly get/set the value for that property. If a get/set method is explicitly defined, it should not be removed. We've been threatening to remove these for years now. Time to rip off the band-aid.

Note that a lot of work is in the test suite. I needed to clean them up and explicitly test that many properties don't have accessors anymore to help me find what needed to be changed.

So the basic guideline I tried to follow here was:

If an underlying get/set method was tagged as both @Kroll.method and @Kroll.getProperty (or @Kroll.setProperty) I'd remove the @Kroll.method.
There are still get/set methods exposed to JS, but they should be "unusual" in the sense that they should accept additional arguments (i.e. a getter accepts some args, even optionally; a setter accepts two+ args)
- For example Ti.Network.HTTPClient.setRequestHeader('headerName', 'value');
In a couple cases I left the getter/setter if we explicitly documented them and hadn't yet deprecated them (and they had properties you could access). I added deprecation notes in the api docs so we can eventually remove them.
For QuickSettingsService it had getter/setter pairs for underlying properties but did not expose the properties. In that case, I exposed the properties, documented them and left the getter/setters but marked them as deprecated in api docs.
I did not remove the deprecation messages baked into Android's proxy code
- I assume we'll want to. I think at this point it's likely to just spit out false reports?

build · 2020-12-02T21:24:16Z

	Warnings
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L22 - tests/Resources/ti.ui.tabgroup.test.js line 22 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L22 - tests/Resources/ti.ui.tabgroup.test.js line 22 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L399 - tests/Resources/ti.ui.tabgroup.test.js line 399 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L402 - tests/Resources/ti.ui.tabgroup.test.js line 402 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L402 - tests/Resources/ti.ui.tabgroup.test.js line 402 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L427 - tests/Resources/ti.ui.tabgroup.test.js line 427 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L428 - tests/Resources/ti.ui.tabgroup.test.js line 428 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L428 - tests/Resources/ti.ui.tabgroup.test.js line 428 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L428 - tests/Resources/ti.ui.tabgroup.test.js line 428 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L429 - tests/Resources/ti.ui.tabgroup.test.js line 429 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L429 - tests/Resources/ti.ui.tabgroup.test.js line 429 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L465 - tests/Resources/ti.ui.tabgroup.test.js line 465 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L468 - tests/Resources/ti.ui.tabgroup.test.js line 468 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.tabgroup.test.js#L468 - tests/Resources/ti.ui.tabgroup.test.js line 468 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L21 - tests/Resources/ti.ui.window.test.js line 21 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
⚠️	tests/Resources/ti.ui.window.test.js#L21 - tests/Resources/ti.ui.window.test.js line 21 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
⚠️	tests/Resources/ti.ui.window.test.js#L696 - tests/Resources/ti.ui.window.test.js line 696 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.window.test.js#L699 - tests/Resources/ti.ui.window.test.js line 699 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L699 - tests/Resources/ti.ui.window.test.js line 699 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L718 - tests/Resources/ti.ui.window.test.js line 718 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.window.test.js#L719 - tests/Resources/ti.ui.window.test.js line 719 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L719 - tests/Resources/ti.ui.window.test.js line 719 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L719 - tests/Resources/ti.ui.window.test.js line 719 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.window.test.js#L720 - tests/Resources/ti.ui.window.test.js line 720 – 'e' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)
⚠️	tests/Resources/ti.ui.window.test.js#L720 - tests/Resources/ti.ui.window.test.js line 720 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L720 - tests/Resources/ti.ui.window.test.js line 720 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L752 - tests/Resources/ti.ui.window.test.js line 752 – Each then() should return a value or throw (promise/always-return)
⚠️	tests/Resources/ti.ui.window.test.js#L755 - tests/Resources/ti.ui.window.test.js line 755 – Avoid nesting promises. (promise/no-nesting)
⚠️	tests/Resources/ti.ui.window.test.js#L755 - tests/Resources/ti.ui.window.test.js line 755 – Avoid nesting promises. (promise/no-nesting)

	Messages
📖	✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖	✅ All tests are passing Nice one! All 15074 tests are passing. (There are 906 skipped tests not included in that total)
📖	👍 Hey!, You deleted more code than you added. That's awesome!

Generated by 🚫 dangerJS against b256dce

garymathews · 2021-02-10T20:04:44Z

There's still some @Kroll.method entries in:

Ti.Calendar.RecurrenceRule

We may also want to deprecate these methods:

Ti.Database.ResultSet.isValidRow()
Ti.Filesystem.isExternalStoragePresent()
Ti.Network.HTTPClient.getAllResponseHeaders() NOTE: This is not documented. The property also need documenting, maybe we can outright remove the getter method?
Ti.Media.AudioPlayer.getAudioSessionId()
Ti.UI.Clipboard.getText() NOTE: Expose properties for other data types? (e.g: Ti.UI.Clipboard.data)
Ti.Codec.getNativeByteOrder()

I see you deprecated some of them in our docs, but we can also add the @Deprecated annotation.

apidoc/Titanium/Network/HTTPClient.yml

… property access

… code template

…classses

…aders()

… method in example

…on properties

…y property

…ovals

…vice

…ndroid since 0.8

sgtcoolguy · 2021-02-10T20:59:13Z

There's still some @Kroll.method entries in:

Ti.Calendar.RecurrenceRule

OK, I will push removal of the RecurrenceRule getters

We may also want to deprecate these methods:

Ti.Database.ResultSet.isValidRow()

Ti.Filesystem.isExternalStoragePresent()

I think these fall into the category of being existing documented methods. We may want to consider exposing as properties and deprecating long-term?

Ti.Network.HTTPClient.getAllResponseHeaders() NOTE: This is not documented. The property also need documenting, maybe we can outright remove the getter method?

As noted in comments w/ Josh, this method should stay. I just missed documenting that Android has had it around since forever. I'll push that change to the docs.

Ti.Media.AudioPlayer.getAudioSessionId()

We didn't expose this as a property before, so I am deprecating it for now but not removing it.

Ti.UI.Clipboard.getText() NOTE: Expose properties for other data types? (e.g: Ti.UI.Clipboard.data)

Given the nature of the API it felt odd to me to try and make it property only for text (but you'd need to use get/set for "data").

Ti.Codec.getNativeByteOrder()

This method has been around forever and is documented. We could consider deprecating and pushing towards a property access usage. But we haven't documented it as a property ever - and I'm trying to only remove "auto-generated" getter/setters for documented properties but retain existing documented methods when possible.

So I think it may make sense to expose as a property here and eventually deprecate the method to prefer the property.

I see you deprecated some of them in our docs, but we can also add the @Deprecated annotation.

vijaysingh-axway

iOS looks good.

sgtcoolguy added ios android docs work in progress 🚧 BREAKING CHANGES ⚠️ test PR that updates our test cases labels Dec 2, 2020

build added this to the 10.0.0 milestone Dec 2, 2020

build requested review from a team December 2, 2020 21:24

build added the needs jira 🚨 label Dec 2, 2020

sgtcoolguy force-pushed the accessors branch 4 times, most recently from 644c910 to b9ca4f8 Compare December 3, 2020 18:18

sgtcoolguy mentioned this pull request Dec 3, 2020

Test Suite clean up #12313

Closed

sgtcoolguy force-pushed the accessors branch 3 times, most recently from b340a28 to 5ba7a01 Compare December 7, 2020 17:53

sgtcoolguy mentioned this pull request Feb 4, 2021

fix(tests): move to getter/setter #12441

Merged

sgtcoolguy force-pushed the accessors branch 3 times, most recently from ae8ad50 to 300f922 Compare February 8, 2021 19:52

sgtcoolguy marked this pull request as ready for review February 10, 2021 16:37

sgtcoolguy removed the work in progress 🚧 label Feb 10, 2021

sgtcoolguy force-pushed the accessors branch from 42f1403 to 64139fc Compare February 10, 2021 18:45

sgtcoolguy added deprecation and removed needs jira 🚨 labels Feb 10, 2021

jquick-axway suggested changes Feb 10, 2021

View reviewed changes

apidoc/Titanium/Network/HTTPClient.yml Show resolved Hide resolved

sgtcoolguy added 21 commits February 10, 2021 15:42

fix(android)!: remove get/set methods for properties that have normal…

ea48771

… property access

fix(android)!: remove auto-generated accessor methods in proxy native…

e16c870

… code template

fix(android)!: remove accessor methods for properties

4dae84b

fix(ios): expose Ti.Network.HTTPClient#getAllResponseHeaders()

25e2113

fix(ios)!: begin ripping out automatic getter/setters for properties

2d99a46

fix(ios)!: don't expose accessors for properties with Obj-c proxy sub…

1e9aefd

…classses

docs(api): explicitly document Ti.Network.HTTPClient#getAllResponseHe…

6d28bde

…aders()

docs(api): use remoteBackup property access in place of getter/setter…

77d9700

… method in example

docs(api): remove bad default values documented for Ti.Platform versi…

dab446b

…on properties

test: invert tests for getters/accessors to ensure they don't exist

c3847b8

test: fix namespace inconsistencies in tests

2487749

test: remove generic getter/setter test

3a8a92d

feat(android): expose Ti.Media.AudioPlayer.audioSessionId as read-onl…

122c74b

…y property

test: update audioplayer suite for accessor changes

4e223fe

test: assign to Intent.flags as property, not using setter

93beac4

docs(api): fix AudioPlayer docs for accessor changes/deprecations/rem…

413a29a

…ovals

fix(android): expose properties for get/set pairs on QuickSettingsSer…

b0a7a99

…vice

docs(api): document QuickSettingsService properties

b4e8cec

fix(android): remove set methods for width/height/center for Views

903ee1a

fix(android): add back Ti.Media.Sound.setLooping for now

b6cfd63

docs(api): mark Ti.Media.Sound.setLooping and setPaused as deprecated

b63364c

sgtcoolguy force-pushed the accessors branch from e6a39cd to b63364c Compare February 10, 2021 20:42

sgtcoolguy added 2 commits February 10, 2021 15:49

docs(api): note Ti.Network.HTTPClient.getAllResponseHeaders() is on A…

f0f5a50

…ndroid since 0.8

fix(android): remove getters for documented properties on RecurrenceRule

b256dce

vijaysingh-axway approved these changes Feb 10, 2021

View reviewed changes

sgtcoolguy merged commit a413a9e into tidev:master Feb 12, 2021

sgtcoolguy deleted the accessors branch February 12, 2021 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Accessors #12310

Remove Accessors #12310

sgtcoolguy commented Dec 2, 2020 •

edited

build commented Dec 2, 2020 •

edited

garymathews commented Feb 10, 2021 •

edited

sgtcoolguy commented Feb 10, 2021

vijaysingh-axway left a comment

Remove Accessors #12310

Remove Accessors #12310

Conversation

sgtcoolguy commented Dec 2, 2020 • edited

build commented Dec 2, 2020 • edited

garymathews commented Feb 10, 2021 • edited

sgtcoolguy commented Feb 10, 2021

vijaysingh-axway left a comment

Choose a reason for hiding this comment

sgtcoolguy commented Dec 2, 2020 •

edited

build commented Dec 2, 2020 •

edited

garymathews commented Feb 10, 2021 •

edited