Skip to content

Conversation

@nikhilrayaprolu
Copy link
Member

In reference to #2692 @niranjan94

},
locationName: function () {
return this.event.location_name.split(",")[0];
if(this.event.location_name){
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should also check for non-empty string. This checks only for the None. Non-empty and undefined should also be checked I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

split works well with non-empty string,and since we are retrieving from model and we defined the attribute location_name as a String and initialising it to None , there is no choice for it to be returned as undefined. @SaptakS
screenshot from 2016-12-24 12-22-18

if(this.event.location_name){
return this.event.location_name.split(",")[0];
}
else{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow the indentation rule of jslint to put else on the same line as closing bracket of if. Also provide spaces before opening brackets.

return this.event.location_name.split(",")[0];
}
else{
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think returning a blank string necessary either. Please check.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 74.57% when pulling ef49f10 on nikhilrayaprolu:eventboxerror into 1585d6f on fossasia:development.

@nikhilrayaprolu
Copy link
Member Author

@SaptakS made a new commit

},
locationName: function () {
return this.event.location_name.split(",")[0];
if (this.event.location_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this check for both null and undefined. Did you try that?

Copy link
Member

Choose a reason for hiding this comment

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

@nikhilrayaprolu also, we need to return a default value ... Since this is a computed value

},
locationName: function () {
return this.event.location_name.split(",")[0];
if (this.event.location_name) {
Copy link
Member

Choose a reason for hiding this comment

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

@nikhilrayaprolu also, we need to return a default value ... Since this is a computed value

@codecov-io
Copy link

codecov-io commented Dec 25, 2016

Current coverage is 74.82% (diff: 100%)

No coverage report found for development at 3b92b78.

Powered by Codecov. Last update 3b92b78...91ceb43

@niranjan94 niranjan94 merged commit df487e7 into fossasia:development Dec 25, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 74.575% when pulling 8cc5fa0 on nikhilrayaprolu:eventboxerror into 3b92b78 on fossasia:development.

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.

6 participants