Skip to content
This repository has been archived by the owner on Nov 26, 2020. It is now read-only.

Removing forced unwrapping #297

Merged
merged 4 commits into from
Jan 12, 2018

Conversation

rodrigorsa
Copy link
Contributor

No description provided.

@rodrigorsa rodrigorsa force-pushed the housekeeping/removing-forced-unwrapping branch from 00f7721 to 0db0713 Compare January 11, 2018 05:31
Copy link
Member

@hebertialmeida hebertialmeida left a comment

Choose a reason for hiding this comment

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

I have just added a question, let me know what do you think.

}

var activeClass: String {
guard let className = metadata.findMetaByProperty("media:active-class") else {
guard let className = metadata.find(byProperty: "media:active-class")?.value else {
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why use .value here and not wrap it in the function as it was before.

Copy link
Contributor Author

@rodrigorsa rodrigorsa Jan 12, 2018

Choose a reason for hiding this comment

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

Since metadata is an object it does not make sense a method named findMetaByProperty() or find(byProperty:) returns a String. In the previous implementation there were two methods named findMetaByName and findMetaByProperty and they were returning different "contents" that is weird. I guess now these method are more consistent and the developer can choose witch Meta's property is better to use

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense.

return "epub-media-overlay-active"
}
return className
}

var playbackActiveClass: String {
guard let className = metadata.findMetaByProperty("media:playback-active-class") else {
guard let className = metadata.find(byProperty: "media:playback-active-class")?.value else {
Copy link
Member

Choose a reason for hiding this comment

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

Same here and in the others that use .value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same as above

@hebertialmeida hebertialmeida self-assigned this Jan 12, 2018
}

var activeClass: String {
guard let className = metadata.findMetaByProperty("media:active-class") else {
guard let className = metadata.find(byProperty: "media:active-class")?.value else {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense.

@hebertialmeida hebertialmeida merged commit 6049777 into master Jan 12, 2018
@hebertialmeida hebertialmeida deleted the housekeeping/removing-forced-unwrapping branch January 12, 2018 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants