Add extension of UIlabel for adding line spacing with the method of n… by osama10 · Pull Request #438 · Esqarrouth/EZSwiftExtensions

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

    }
+
+    // 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 <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 · 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

+    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

+        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

+        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

   - `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


 ## 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 <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 - + **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

+        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

 //

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


You have unresolved merge commits here.

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

        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

+    @testable import EZSwiftExtensions
+    class UILabelTests: XCTestCase {

-        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

		label.setLineSpacing(lineSpacing: 1.5)

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

Conversation

osama10 commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

EZSwiftExtensionsBot commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Khalian commented Jul 11, 2017

Uh oh!

osama10 commented Jul 11, 2017 via email

Uh oh!

Khalian commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osama10 commented Jul 13, 2017 via email

Uh oh!

Khalian commented Jul 13, 2017

Uh oh!

osama10 commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

osama10 commented Aug 6, 2017

Uh oh!

Khalian commented Aug 6, 2017

Uh oh!

lfarah commented Aug 6, 2017

Uh oh!

osama10 commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Khalian commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Khalian Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

osama10 commented Aug 30, 2017

Uh oh!

Khalian commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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