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

Add new computed-property-getters rule #397

Merged
merged 3 commits into from May 8, 2019

Conversation

jrjohnson
Copy link
Contributor

@jrjohnson jrjohnson commented Mar 22, 2019

Adds rule to fix #344

Which has options to never allow getters in computed properties or always require them.

/// GOOD
Ember.Object.extend({
    fullName: computed('firstName', 'lastName', function() {
        //...
    })
});

// BAD
Ember.Object.extend({
    fullName: computed('firstName', 'lastName', {
        get() {
            //...
        }
    })
});

@Turbo87
Copy link
Member

Turbo87 commented Mar 22, 2019

#344 discussed making the rule configurable to allow it to be reversed. in that case, calling the rule no-computed-property-getters might be misleading. having the rule without config seems like a good first step to me, but we should ensure that it doesn't have to be renamed if we ever added configuration.

Defaults to `never` allow getters in computed properties, has the option
to `always` require them.
@jrjohnson jrjohnson changed the title Add rule to prevent computer property getters Add rule to enforce computed property style Mar 23, 2019
@jrjohnson
Copy link
Contributor Author

Thanks @Turbo87. I lost track of that requirement. I was able to figure out how to create the options and test them so now this rule supports both.

@jrjohnson
Copy link
Contributor Author

@Turbo87 just checking in on this to see if there is anything I should change before I completely forget the context.


String option:

* `"never"` (default) getters are *never allowed* in computed properties
Copy link
Member

Choose a reason for hiding this comment

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

I think the default should be to only allow them if there is also a setter present. don't ask me how to call that option... 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I started working on the docs for a change to add always-with-setter as an option here it strikes me as a strange thing to lint against. A property created like this would throw an error when accessed anyway since there is no way to get it's value.

Ember.Object.extend({
    fullName: computed('firstName', 'lastName', {
        set() {
            //...
        }
    })
});

Am I confused about what your comment is asking for?

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is:

// good
Ember.Object.extend({
    fullName: computed('firstName', 'lastName', function() {
        //...
    })
});

// bad
Ember.Object.extend({
    fullName: computed('firstName', 'lastName', {
        get() {
            //...
        }
    })
});

// good
Ember.Object.extend({
    fullName: computed('firstName', 'lastName', {
        get() {
            //...
        },
        set() {
            //...
        }
    })
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification @Turbo87, I've captured that now and updated the default.

@Turbo87
Copy link
Member

Turbo87 commented Apr 26, 2019

@jrjohnson it looks like CI is unhappy about the latest change 🤔

This defaults to requiring getters if there is a setter. It preserves
the option to prevent getters entirely as a proxy for preventing setters
as well.
@jrjohnson
Copy link
Contributor Author

Whoops, thanks @Turbo87. Fixed now.

A setter should never be present when there are no getters no matter
what other options are selected.
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

👏 nice work!

@Turbo87 Turbo87 merged commit cce30db into ember-cli:master May 8, 2019
@Alonski
Copy link
Contributor

Alonski commented May 8, 2019

Awesome work @jrjohnson Thank you so much

@jrjohnson jrjohnson deleted the 344-computed-style branch May 8, 2019 16:27
@jrjohnson
Copy link
Contributor Author

🎉 thanks for all the great feedback.

@bmish bmish changed the title Add rule to enforce computed property style Add new computed-property-getters rule May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: computed-property-style
3 participants