Resolves #92, Identifier Name Violation Issues after updating to Xcode 9.3 #93

antsangrigoli · 2018-05-04T19:14:35Z

PR Description

This is in response to issue #92. When building the project in Xcode 9.3, I was getting 9 Identifier Name Violations errors. I've fixed these errors by adding // swiftlint:disable:next identifier_name as you can see in the screenshots below. Project can now be built and ran in Xcode 9.3.

Type of Changes

Fixes Issue Identifier Name Violation Issues and Warning after updating to Xcode 9.3 #92

Screenshots

Before

After

Checklist

Make sure you've done all the following (Put an x in the boxes that apply.)

If you have multiple commits please combine them into one commit by squashing them.
Code is well documented
Included unit tests for new functionality
All user-visible strings are made translatable
Code passes Travis builds in your branch

rudrankriyam · 2018-05-04T19:19:32Z

Sources/ViewControllers/Settings/SettingsTableViewController.swift

@@ -32,6 +32,7 @@ class SettingsTableViewController: UITableViewController, MFMailComposeViewContr
    }

    override func tableView(_ tableView: UITableView, titleForHeaderInSection section: Int) -> String? {
+        // swiftlint:disable:next identifier_name


Actually, instead of disabling swiftlint here, the 'Settings' variable should be 'settings' variable.

antsangrigoli

changed variable Settings to settings

rudrankriyam · 2018-05-04T19:39:38Z

I had two solution for this -

Either add swiftlint disable rule, or
Change the name of the variable.

Like, there are many warnings due to 'vc' variable, referring to ViewControllers. We can rename them to SummaryFormTableVC or NutritionTableFormVC etc

What do you propose @antsangrigoli @roger-tan ?

roger-tan · 2018-05-04T20:42:38Z

Sources/Extensions/UIColor.swift

@@ -19,10 +19,12 @@ extension UIColor {

        var rgb: UInt32 = 0

+        // swiftlint:disable identifier_name
        var r: CGFloat = 0.0


Personally, I'm not a huge fan of abbreviation. Instead to disable the error. We should just rename r as red, green as green, b as blue and a as alpha. This will make the code understandable without guessing each abbreviation what does it means.

antsangrigoli

Renamed variables r to red, g to green, b to blue, a to alpha in UIColor.swift

@snuff4 @roger-tan
The only other warnings I can see about an Identifier Name Violation are for the variables "id" and "vc".
As snuff4 mentioned, I'm going to rename each "vc" variable to its specific view controller name

antsangrigoli

Renamed each "vc" variable to its specific view controller name

aleene · 2018-05-05T06:28:04Z

I concur. I prefer large variable names as well, so it is clear what a variable represents.

By the way, if I have an optional, which I unwrap with guard or let, I start the new variable with validVariable, so I know the original was an optional.

rudrankriyam · 2018-05-05T15:08:49Z

Sources/ViewControllers/User/UserViewController.swift

@@ -31,28 +31,28 @@ class UserViewController: UIViewController, DataManagerClient {
                showProductsPendingUpload()
            }

-            vc = childNavigationController
+            UIVC = childNavigationController



Can you change it to childNavigationVC? And we need to make sure every function of the app is working, after the changes

antsangrigoli

Renamed "UIVC" to childNavigationVC

rudrankriyam · 2018-05-09T22:05:08Z

@antsangrigoli can you ensure that everything is working after renaming?

antsangrigoli · 2018-05-09T23:31:06Z

@snuff4 yes everything seems to work fine.
However, I'm getting warnings on the variable 'id'. You think I should add the swiftlint disable rule or rename 'id' ?

rudrankriyam · 2018-05-09T23:36:31Z

@antsangrigoli I saw that too. There's a issue related to this, too.

antsangrigoli · 2018-05-10T00:39:05Z

@snuff4 Just read up on it. I could add the following parameters below to the .swiftlint.yml file and this will solve the issue.

identifier_name:
excluded:
- id

rudrankriyam · 2018-05-10T17:28:32Z

poke @roger-tan For this I think we can exclude for this. We need to finish with this PR asap, because without it, it's not working on 9.3

roger-tan · 2018-05-10T17:40:20Z

Sources/Extensions/UIViewController.swift

@@ -29,8 +29,8 @@ extension UIViewController {
    }

    func openUrlInApp(_ url: URL, showAlert: Bool? = nil) {
-        let vc = SFSafariViewController(url: url, entersReaderIfAvailable: false)
-        present(vc, animated: true)
+        let SFSafariVC = SFSafariViewController(url: url, entersReaderIfAvailable: false)


Personally for local context. I would just call controller but a variable name cannot with an uppercase letter. We should keep the camel case for a name

So I should rename the variable SFSafariVC to sfSafariVC ?

To keep consistence inside the projet. Just sfSafariVC.

roger-tan · 2018-05-10T17:41:20Z

For id. It's ok for me.

antsangrigoli

Renamed SFSafariVC to sfSafariVC
Added parameter in .swiftlint.yml file to exclude variable name "id"

roger-tan · 2018-05-12T07:46:12Z

@antsangrigoli Thank you for your contributions into OpenFoodFacts. Your Pull Request is now merged .

Added swiftlint:disable:next identifier_name

ee1191b

rudrankriyam suggested changes May 4, 2018

View reviewed changes

Changed variable from Settings to settings

c74f399

antsangrigoli commented May 4, 2018

View reviewed changes

rudrankriyam requested a review from roger-tan May 4, 2018 19:30

roger-tan reviewed May 4, 2018

View reviewed changes

Renamed variable r to red, g to green, b to blue, a to alpha

542c689

antsangrigoli commented May 4, 2018

View reviewed changes

Renamed each "vc" variable to its specific view controller name

a51cfeb

antsangrigoli commented May 4, 2018

View reviewed changes

rudrankriyam suggested changes May 5, 2018

View reviewed changes

rudrankriyam mentioned this pull request May 5, 2018

Resolves #88 Add new white icons [WIP] #91

Merged

Renamed "UIVC" to childNavigationVC

04302f2

antsangrigoli commented May 5, 2018

View reviewed changes

rudrankriyam added the bug label May 9, 2018

roger-tan suggested changes May 10, 2018

View reviewed changes

Renamed SFSafariVC to sfSafariVC. Also excluded "id" in .swiftlint file

08449a3

antsangrigoli commented May 11, 2018

View reviewed changes

rudrankriyam approved these changes May 11, 2018

View reviewed changes

roger-tan approved these changes May 12, 2018

View reviewed changes

roger-tan merged commit 0aff145 into openfoodfacts:master May 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolves #92, Identifier Name Violation Issues after updating to Xcode 9.3 #93

Resolves #92, Identifier Name Violation Issues after updating to Xcode 9.3 #93

antsangrigoli commented May 4, 2018

rudrankriyam May 4, 2018

antsangrigoli left a comment

rudrankriyam commented May 4, 2018 •

edited

Loading

roger-tan May 4, 2018 •

edited

Loading

antsangrigoli left a comment •

edited

Loading

antsangrigoli left a comment

aleene commented May 5, 2018 •

edited

Loading

rudrankriyam May 5, 2018 •

edited

Loading

antsangrigoli left a comment

rudrankriyam commented May 9, 2018

antsangrigoli commented May 9, 2018

rudrankriyam commented May 9, 2018

antsangrigoli commented May 10, 2018

rudrankriyam commented May 10, 2018

roger-tan May 10, 2018

antsangrigoli May 10, 2018

roger-tan May 11, 2018

roger-tan commented May 10, 2018

antsangrigoli left a comment •

edited

Loading

roger-tan commented May 12, 2018

Resolves #92, Identifier Name Violation Issues after updating to Xcode 9.3 #93

Resolves #92, Identifier Name Violation Issues after updating to Xcode 9.3 #93

Conversation

antsangrigoli commented May 4, 2018

PR Description

Screenshots

Before

After

Checklist

rudrankriyam May 4, 2018

Choose a reason for hiding this comment

antsangrigoli left a comment

Choose a reason for hiding this comment

rudrankriyam commented May 4, 2018 • edited Loading

roger-tan May 4, 2018 • edited Loading

Choose a reason for hiding this comment

antsangrigoli left a comment • edited Loading

Choose a reason for hiding this comment

antsangrigoli left a comment

Choose a reason for hiding this comment

aleene commented May 5, 2018 • edited Loading

rudrankriyam May 5, 2018 • edited Loading

Choose a reason for hiding this comment

antsangrigoli left a comment

Choose a reason for hiding this comment

rudrankriyam commented May 9, 2018

antsangrigoli commented May 9, 2018

rudrankriyam commented May 9, 2018

antsangrigoli commented May 10, 2018

rudrankriyam commented May 10, 2018

roger-tan May 10, 2018

Choose a reason for hiding this comment

antsangrigoli May 10, 2018

Choose a reason for hiding this comment

roger-tan May 11, 2018

Choose a reason for hiding this comment

roger-tan commented May 10, 2018

antsangrigoli left a comment • edited Loading

Choose a reason for hiding this comment

roger-tan commented May 12, 2018

rudrankriyam commented May 4, 2018 •

edited

Loading

roger-tan May 4, 2018 •

edited

Loading

antsangrigoli left a comment •

edited

Loading

aleene commented May 5, 2018 •

edited

Loading

rudrankriyam May 5, 2018 •

edited

Loading

antsangrigoli left a comment •

edited

Loading