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

osama10 · 2017-07-11T08:03:44Z

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

New Extension
New Test
Changed more than one extension, but all changes are related
Trivial change (doesn't require changelog)

…ame setLineSpacing

EZSwiftExtensionsBot · 2017-07-11T08:12:32Z

	1 Error
🚫	Please rebase to get rid of the merge commits in this PR

	2 Warnings
⚠️	EZSwiftExtensionsTests/UILabelTests.swift#L56: treating a forced downcast to ‘NSMutableParagraphStyle’ as optional will never produce ‘nil’ `paragraphStyle = value as! NSMutableParagraphStyle`
⚠️	EZSwiftExtensionsTests/UILabelTests.swift#L56: treating a forced downcast to ‘NSMutableParagraphStyle’ as optional will never produce ‘nil’ `paragraphStyle = value as! NSMutableParagraphStyle`

	3 Messages
📖	Executed 202 tests, with 0 failures (0 unexpected) in 7.126 (7.382) seconds
📖	Executed 187 tests, with 0 failures (0 unexpected) in 9.344 (9.549) seconds
📖	Executed 123 tests, with 0 failures (0 unexpected) in 4.724 (4.832) seconds

Generated by 🚫 Danger

…ame setLineSpacing

Khalian · 2017-07-11T17:16:16Z

Sources/UILabelExtensions.swift

@@ -65,6 +65,19 @@ extension UILabel {
            self.text = _text
        }, completion: nil)
    }
+
+    // Set lineSpacing for UILabel
+    public func setLineSpacing(lineSpacing: CGFloat) {


Is it possible to write a unit test for this ?

Khalian · 2017-07-11T17:16:32Z

Could you also add a changelog entry for this. ?

osama10 · 2017-07-11T17:27:19Z

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 <[email protected]> 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 · 2017-07-11T20:00:24Z

@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

Khalian · 2017-07-12T03:48:12Z

Sources/UILabelExtensions.swift

+    public func setLineSpacing(lineSpacing: CGFloat) {
+        let paragraphStyle = NSMutableParagraphStyle()
+        paragraphStyle.lineSpacing = 1.0
+        paragraphStyle.lineHeightMultiple = lineSpacing


This is confusing. Does this mean your spacing = 1.0 * lineHeight = lineSpacing. If so, you could just call the method arg as lineHeightMultiple.

Khalian · 2017-07-12T03:49:16Z

Sources/UILabelExtensions.swift

+        paragraphStyle.lineHeightMultiple = lineSpacing
+        paragraphStyle.alignment = self.textAlignment
+
+        let attrString = NSMutableAttributedString(string: self.text!)


This could actually live as its own var. Otherwise its unnecessary allocation of the attr string per call.

Khalian · 2017-07-12T03:49:52Z

Sources/UILabelExtensions.swift

+        paragraphStyle.alignment = self.textAlignment
+
+        let attrString = NSMutableAttributedString(string: self.text!)
+        attrString.addAttribute(NSFontAttributeName, value: self.font, range: NSMakeRange(0, attrString.length))


NSMakeRange is repeated, you can abstract that away as a single call.

Khalian · 2017-07-12T17:04:22Z

CHANGELOG.md

@@ -54,6 +54,9 @@ All notable changes to this project will be documented in this file.
   - `validateEmail()` in [[PR]](https://github.com/goktugyil/EZSwiftExtensions/pull/429) by *Vic-L*
   - `validateDigits()` in [[PR]](https://github.com/goktugyil/EZSwiftExtensions/pull/429) by *Vic-L*

+12. **UILabelExtensions**
+   - `setLineSpacing(lineSpacing : CGFloat)` in [[PR]](https://github.com/goktugyil/EZSwiftExtensions/pull/438) by *osama10* 


Erm not under v1.10. Under unreleased. V 1.10 is already released as a pod.

codecov-io · 2017-07-13T08:58:44Z

Codecov Report

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

@@            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.

Khalian · 2017-07-13T17:17:52Z

CHANGELOG.md

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

 ## Unreleased
-
+ **UILabelExtensions**


Minor : With a number (in this case 1).

So do I need removed this spacing ??

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.

osama10 · 2017-07-13T18:13:53Z

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 <[email protected]> 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 - + **UILabelExtensions** 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 · 2017-07-13T18:51:21Z

"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 · 2017-08-06T07:43:22Z

@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 · 2017-08-06T07:45:22Z

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

Khalian · 2017-08-06T09:05:37Z

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"

There are a bunch of comments I posted.

lfarah · 2017-08-06T22:04:24Z

Hey @osama10, take a look at this

osama10 · 2017-08-25T08:04:36Z

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

Khalian · 2017-08-25T19:54:21Z

EZSwiftExtensionsTests/UILabelTests.swift

+        label.setLineSpacing(lineSpacing: 1.5)
+
+        label.attributedText?.enumerateAttribute(NSParagraphStyleAttributeName , in: NSMakeRange(0, (label.attributedText?.length)!), options: [.longestEffectiveRangeNotRequired]) { value, range, isStop in
+


nitpick : too many empty lines here are there that do not serve a purpose.

Khalian · 2017-08-25T19:54:56Z

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

# Conflicts: # EZSwiftExtensionsTests/UILabelTests.swift

Khalian · 2017-08-30T05:55:35Z

EZSwiftExtensionsTests/UILabelTests.swift

@@ -7,6 +7,8 @@
 //

 #if os(iOS) || os(tvOS)
+<<<<<<< HEAD


You have unresolved merge commits here.

Khalian · 2017-08-30T05:59:16Z

EZSwiftExtensionsTests/UILabelTests.swift

@@ -22,34 +24,67 @@ class UILabelTests: XCTestCase {
        XCTAssertEqual(label2.font.pointSize, 20)
        XCTAssertEqual(label.font.pointSize, 17)
    }
+>>>>>>> 638c47200b16776d978dc4598237109396915878


This looks like an unresolved merge, please collapse your commits into one idempotent change.

I cannot even begin to tell you how much I despise non fast forward merges. Rebase your changes and replay them on top of the current HEAD commit.

Khalian · 2017-08-30T06:40:39Z

EZSwiftExtensionsTests/UILabelTests.swift


-        let label = UILabel(x: 0, y: 0, w: 200, h: 50)


Are there any changes in these parts? Please make the code change idempotent to only the changes you are making. This looks like an indentation change which is unnecessary. The code already was properly indented.

Another important part of making this change idempotent is to ensure it appears as one single commit in the PR. I do not want to manually merge 12 commits with a couple of merge commits in it.

osama10 · 2017-08-30T09:23:32Z

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

Khalian · 2017-08-30T16:21:46Z

@osama10

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.
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.
You haven't actually addressed any of my comments sans the unit tests.

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

cd810e5

…ame setLineSpacing

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

6a55e8f

…ame setLineSpacing

Khalian reviewed Jul 11, 2017

View reviewed changes

Khalian reviewed Jul 12, 2017

View reviewed changes

osama10 added 2 commits July 12, 2017 17:51

Update CHANGELOG.md

f949239

Update CHANGELOG.md

d4b9474

Khalian reviewed Jul 12, 2017

View reviewed changes

Update CHANGELOG.md

2d20dac

Khalian reviewed Jul 13, 2017

View reviewed changes

osama10 added 3 commits August 25, 2017 12:58

Added testForLineSpacing

d59ca01

Merge branch 'master' of https://github.com/osama10/EZSwiftExtensions

c4d58e0

Merge branch 'master' into master

638c472

Khalian reviewed Aug 25, 2017

View reviewed changes

osama10 added 2 commits August 28, 2017 17:16

Removed unnecessary linespacing in testLineSpacing function

61f701b

Merge branch 'master' of https://github.com/osama10/EZSwiftExtensions

e5f55c5

# Conflicts: # EZSwiftExtensionsTests/UILabelTests.swift

Khalian reviewed Aug 30, 2017

View reviewed changes

osama10 added 2 commits August 30, 2017 11:28

Update the UILabel test class

c00d920

Update CHANGELOG.md

277316f

Khalian reviewed Aug 30, 2017

View reviewed changes

osama10 added 4 commits August 29, 2017 23:48

Merge branch 'master' into master

fac501a

Update CHANGELOG.md

13b1dcc

Update the testOfLineSpacing

f3047f8

Rebased

e736d02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

osama10 commented Jul 11, 2017 •

edited

Loading

EZSwiftExtensionsBot commented Jul 11, 2017 •

edited

Loading

Khalian Jul 11, 2017

Khalian commented Jul 11, 2017

osama10 commented Jul 11, 2017 via email

Khalian commented Jul 11, 2017 •

edited

Loading

Khalian Jul 12, 2017

Khalian Jul 12, 2017

Khalian Jul 12, 2017

Khalian Jul 12, 2017

codecov-io commented Jul 13, 2017 •

edited

Loading

Khalian Jul 13, 2017

osama10 Jul 13, 2017

Khalian Jul 13, 2017

osama10 commented Jul 13, 2017 via email

Khalian commented Jul 13, 2017

osama10 commented Aug 6, 2017 •

edited

Loading

osama10 commented Aug 6, 2017

Khalian commented Aug 6, 2017

lfarah commented Aug 6, 2017

osama10 commented Aug 25, 2017

Khalian Aug 25, 2017

Khalian commented Aug 25, 2017

Khalian Aug 30, 2017

Khalian Aug 30, 2017

Khalian Aug 30, 2017 •

edited

Loading

osama10 commented Aug 30, 2017

Khalian commented Aug 30, 2017

		label.setLineSpacing(lineSpacing: 1.5)

		label.attributedText?.enumerateAttribute(NSParagraphStyleAttributeName , in: NSMakeRange(0, (label.attributedText?.length)!), options: [.longestEffectiveRangeNotRequired]) { value, range, isStop in

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

Are you sure you want to change the base?

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

Conversation

osama10 commented Jul 11, 2017 • edited Loading

Checklist

EZSwiftExtensionsBot commented Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Khalian commented Jul 11, 2017

osama10 commented Jul 11, 2017 via email

Khalian commented Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 13, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osama10 commented Jul 13, 2017 via email

Khalian commented Jul 13, 2017

osama10 commented Aug 6, 2017 • edited Loading

osama10 commented Aug 6, 2017

Khalian commented Aug 6, 2017

lfarah commented Aug 6, 2017

osama10 commented Aug 25, 2017

Choose a reason for hiding this comment

Khalian commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Khalian Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

osama10 commented Aug 30, 2017

Khalian commented Aug 30, 2017

osama10 commented Jul 11, 2017 •

edited

Loading

EZSwiftExtensionsBot commented Jul 11, 2017 •

edited

Loading

Khalian commented Jul 11, 2017 •

edited

Loading

codecov-io commented Jul 13, 2017 •

edited

Loading

osama10 commented Aug 6, 2017 •

edited

Loading

Khalian Aug 30, 2017 •

edited

Loading