Esqarrouth / EZSwiftExtensions

:smirk: How Swift standard types and classes were supposed to work.
MIT License
3k stars 381 forks source link

Add extension of UIlabel for adding line spacing with the method of n… #438

Open osama10 opened 7 years ago

osama10 commented 7 years ago

I have added the extension of UILabel for adding linespacing between line of UILabel , it takes CGFloat as input and add that spacing to the UILabel

Checklist

EZSwiftExtensionsBot commented 7 years ago
1 Error
:no_entry_sign: Please rebase to get rid of the merge commits in this PR
2 Warnings
:warning: EZSwiftExtensionsTests/UILabelTests.swift#L56: treating a forced downcast to ‘NSMutableParagraphStyle’ as optional will never produce ‘nil’
paragraphStyle = value as! NSMutableParagraphStyle
:warning: EZSwiftExtensionsTests/UILabelTests.swift#L56: treating a forced downcast to ‘NSMutableParagraphStyle’ as optional will never produce ‘nil’
paragraphStyle = value as! NSMutableParagraphStyle
3 Messages
:book: Executed 202 tests, with 0 failures (0 unexpected) in 7.126 (7.382) seconds
:book: Executed 187 tests, with 0 failures (0 unexpected) in 9.344 (9.549) seconds
:book: Executed 123 tests, with 0 failures (0 unexpected) in 4.724 (4.832) seconds

Generated by :no_entry_sign: Danger

Khalian commented 7 years ago

Could you also add a changelog entry for this. ?

osama10 commented 7 years ago

Can you please guide me how to do this  I am doing.my first contribution . I'll be very thankful to you if you guide me about thisOn 11-Jul-2017 10:16 pm, Arunav Sanyal notifications@github.com wrote:Could you also add a changelog entry for this. ?

—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.

Khalian commented 7 years ago

@osama10 Sure thing.

The changelog entry is a piece of text in the CHANGELOG.md file. Here is a sample entry

Date init?(httpDateString: String)` in [PR] by Vic-L

The format is ExtensionClass methodAdded in [[PR]] (URL to pull request) by Author

codecov-io commented 7 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@88919e2). Click here to learn what that means. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #438   +/-   ##
=========================================
  Coverage          ?   41.93%           
=========================================
  Files             ?       50           
  Lines             ?     2258           
  Branches          ?        0           
=========================================
  Hits              ?      947           
  Misses            ?     1311           
  Partials          ?        0
Impacted Files Coverage Δ
Sources/UILabelExtensions.swift 36.84% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 88919e2...e736d02. Read the comment docs.

osama10 commented 7 years ago

I am new to swift development and don't know yet about writing unit test yet ... what should I do and is it necessary to write unit test for ensuring the acceptance of my contributionOn 13-Jul-2017 10:56 pm, Arunav Sanyal notifications@github.com wrote:@Khalian commented on this pull request.

In CHANGELOG.md:

@@ -2,7 +2,8 @@ All notable changes to this project will be documented in this file.

Unreleased

-

have a 1 prior to that. Like a numbering thing. But that is really cosmetic, I would suggest taking a look at writing unit tests for the same.

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

Khalian commented 7 years ago

"I am new to swift development and don't know yet about writing unit test yet ... what should I do"

In order to write unit tests you would have to edit the unit test file, in your case https://github.com/goktugyil/EZSwiftExtensions/blob/master/EZSwiftExtensionsTests/UILabelTests.swift and run that against travis ci build (you can locally test the change on xcode by running the test target we wrote).

"is it necessary to write unit test for ensuring the acceptance of my contributionOn"

A unit test is a contract that gives reasonable confidence on the veracity of your changes. It also provides documentation in the form of self explanatary code, and also what said extension is meant to achieve. Unless its impossible to achieve writing a unit tests (an example would be UIDevice tests were write based mocks are blocked by swifts runtime), we should have unit tests.

osama10 commented 7 years ago

@Khalian It's not possible to write the unit test of my extension as it gives the line spacing between the lines of label and can on be witnessed visually.

osama10 commented 7 years ago

so what should i do now to get my contribution accepted. @Khalian

Khalian commented 7 years ago
  1. You can write a unit test by expecting the mutable state of self.attributedText attribute to contain that line spacing delta. That would verify that the paragraph style is modified (or newly created, this is not really my domain of expertise)

The purpose of the test is to make the method not brittle to change.

"so what should i do now to get my contribution accepted. @Khalian"

  1. There are a bunch of comments I posted.
lfarah commented 7 years ago

Hey @osama10, take a look at this

osama10 commented 7 years ago

@Khalian i have added the test . Now what should i do ??

Khalian commented 7 years ago

I have quite a few comments across the files. Can you take a look at those?

osama10 commented 7 years ago

@Khalian Can you please tell me what is the issue with my commit

Khalian commented 7 years ago

@osama10

  1. You have 16 of them right now instead of one idempotent change (i.e. one single commit). Moreover your commit is messing up with parts of the code that it should not.

  2. You are doing non fast forward merges, which is making the whole commit structure worse. Rebase and replay mainline on your branch, and then squash your commits to one idempotent change.

  3. You haven't actually addressed any of my comments sans the unit tests.