Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use ExtractorBase for Swift #346

Merged
merged 2 commits into from
Sep 19, 2018
Merged

use ExtractorBase for Swift #346

merged 2 commits into from
Sep 19, 2018

Conversation

asurkov
Copy link
Contributor

@asurkov asurkov commented Sep 11, 2018

No description provided.

}
object SwiftExtractor : ExtractorBase(
language = Lang.SWIFT,
importRegex = Regex("""^(.*import)\s[^\n]*"""),
Copy link
Member

@anatolystansler anatolystansler Sep 12, 2018

Choose a reason for hiding this comment

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

extractImportRegex used for actually extracting of imports for choosing right classifiers
importRegex used to filter tokens from imports and as @yaronskaya works slightly faster

Here importRegex (less accurate version) used for both extracting and filter tokens. @yaronskaya review this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anatolystansler Curious, whether it is noticeable at all. Having two nearly identical regexps affects on code readability and its maintenance. So if it's not a perf problem, then I would choose simplicity over optimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

@asurkov this regex doesn't extract imports. Please, use 'extractImportRegex' from previous version of SwiftExtractor. I wonder if it passes tests:)

}
object SwiftExtractor : ExtractorBase(
language = Lang.SWIFT,
importRegex = Regex("""^(.*import)\s[^\n]*"""),
Copy link
Contributor

Choose a reason for hiding this comment

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

@asurkov this regex doesn't extract imports. Please, use 'extractImportRegex' from previous version of SwiftExtractor. I wonder if it passes tests:)

@asurkov
Copy link
Contributor Author

asurkov commented Sep 14, 2018

@yaronskaya now I see what you say. The tests indeed pass, which means it has to be a problem with the tests. For example, there's one val line = "import Foundation", which is supposed to extract no libraries. Do you know whether this is correct one? Is there any special about Swift import instructions?

@yaronskaya
Copy link
Contributor

@asurkov it turned out that we don't have tests of imports for Swift, there are only tests for libraries extraction. Could you please add them?

Copy link
Contributor

@yaronskaya yaronskaya left a comment

Choose a reason for hiding this comment

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

Looks great!
Please address the comment and we will merge it.

"import UIKit"
)
var actualLineImports = SwiftExtractor.extractImports(lines)
assertEquals(actualLineImports, listOf("UIKit"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use assertExtractsImport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants