Merged
Conversation
Member
Author
|
It is releated with #219 |
Checklist 🤔
|
kanghyojun
force-pushed
the
import-aliasing
branch
from
afd38bf to
a69542e
Compare
Codecov Report
@@ Coverage Diff @@
## master #258 +/- ##
==========================================
+ Coverage 75.84% 76.16% +0.31%
==========================================
Files 33 33
Lines 2414 2446 +32
Branches 132 132
==========================================
+ Hits 1831 1863 +32
Misses 451 451
Partials 132 132
Continue to review full report at Codecov.
|
kanghyojun
force-pushed
the
import-aliasing
branch
from
a69542e to
6b824da
Compare
kanghyojun
added
typ:enhance
dahlia
requested changes
Apr 25, 2018
test/Nirum/Package/ModuleSetSpec.hs
Outdated
| , Import ["baz"] "qux" empty | ||
| ] Nothing | ||
| , Module | ||
| [ Import ["foo", "bar"] "xyz" "xyz" empty --MissingModulePathError |
Member
There was a problem hiding this comment.
Please add a space right after --.
| return [Import path ident aSet | (ident, aSet) <- idents] | ||
| return [ Import path source imp aSet | ||
| | (imp, source, aSet) <- idents | ||
| ] |
Member
There was a problem hiding this comment.
We should have tests for all combinations of the following conditions:
| Empty parentheses | Single import name w/o trailing comma | Single import name w/ trailing comma | Multiple import names w/o trailing comma | Multiple import names w/ trailing comma | |
|---|---|---|---|---|---|
With as |
import foo () |
import foo (x as a) |
import foo (x as a,) |
import foo (x as a, y as b, c) |
import foo (x as a, y as b, c,) |
Without as |
import foo() |
import foo (a) |
import foo(a, b, c,) |
import foo (a,) |
import foo(a, b, c) |
Member
There was a problem hiding this comment.
We should test if the parser disallows and shows an error when there are duplicated alias names.
kanghyojun
force-pushed
the
import-aliasing
branch
3 times, most recently
from
1e581a8 to
77b5274
Compare
kanghyojun
force-pushed
the
import-aliasing
branch
from
77b5274 to
25da998
Compare
dahlia
requested changes
Apr 25, 2018
test/Nirum/Constructs/ModuleSpec.hs
Outdated
| , (["zzz"], ["qqq", "ppp"]) | ||
| imports mod1 `shouldBe` [ (["foo", "bar"], [ ("baz", "baz") | ||
| , ("qux", "qux") | ||
| ]) |
test/Nirum/Constructs/ModuleSpec.hs
Outdated
| , (["xyz"], [("asdf", "asdf")]) | ||
| , (["zzz"], [ ("qqq", "qqq") | ||
| , ("ppp", "ppp") | ||
| ]) |
| Nothing -> toType | ||
| coreModulePath | ||
| identifier | ||
| (DS.lookup identifier $ types coreModule) |
test/Nirum/Package/ModuleSetSpec.hs
Outdated
| , Import ["foo"] "def" empty -- MissingImportError | ||
| , (["qux"], Module [ Import ["foo"] "abc" "abc" empty -- MissingImportError | ||
| , Import ["foo"] "def" "def" empty -- MissingImportError | ||
| ] Nothing) |
test/Nirum/Package/ModuleSetSpec.hs
Outdated
| [ (["asdf"], Module [ Import ["asdf"] "foo" empty | ||
| [ (["asdf"], Module [ Import ["asdf"] "foo" "foo" empty | ||
| , TypeDeclaration "bar" (Alias "text") empty | ||
| ] Nothing) |
test/Nirum/Package/ModuleSetSpec.hs
Outdated
| , (["abc", "def"], Module [ Import ["abc", "ghi"] "bar" "bar" empty | ||
| , TypeDeclaration | ||
| "foo" (Alias "text") empty | ||
| ] Nothing) |
test/Nirum/Package/ModuleSetSpec.hs
Outdated
| , (["abc", "xyz"], Module [ Import ["abc", "def"] "foo" "foo" empty | ||
| , TypeDeclaration | ||
| "baz" (Alias "text") empty | ||
| ] Nothing) |
test/Nirum/ParserSpec.hs
Outdated
| let (parse', expectError) = helperFuncs $ P.imports [] | ||
| it "can single import name w/o trailing comma" $ do | ||
| parse' "import foo.bar (a);" `shouldBeRight` | ||
| [ Import ["foo", "bar"] "a" "a" empty ] |
test/Nirum/ParserSpec.hs
Outdated
| parse' "import foo.bar (a,);" `shouldBeRight` | ||
| [ Import ["foo", "bar"] "a" "a" empty ] | ||
| parse' "import foo.bar (a as qux,);" `shouldBeRight` | ||
| [ Import ["foo", "bar"] "qux" "a" empty ] |
src/Nirum/Parser.hs
Outdated
| <?> "names to import" | ||
| idents <- many' [] $ \ importNames' -> do | ||
| notFollowedBy $ choice [char ')', char ',' >> spaces >> char ')'] | ||
| let forwardNames' = [ i | (i, _, _) <- importNames' ] ++ |
kanghyojun
force-pushed
the
import-aliasing
branch
from
25da998 to
1a7da15
Compare
dahlia
requested changes
Apr 25, 2018
Member
dahlia
left a comment
dahlia
left a comment
There was a problem hiding this comment.
Build is failing due to lint:
test/Nirum/TypeInstance/BoundModuleSpec.hs:56:1: too long line (83 chars)
test/Nirum/ParserSpec.hs:1180:1: too long line (81 chars)
test/Nirum/ParserSpec.hs:1185:1: too long line (81 chars)
test/Nirum/ParserSpec.hs:1206:1: too long line (82 chars)
test/Nirum/ParserSpec.hs:1215:1: too long line (83 chars)
test/Nirum/ParserSpec.hs:1255:1: too long line (91 chars)
1a7da15 to
6be588a
Compare
dahlia
approved these changes
Apr 30, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It's handy to avoid a name shadowing. It resolve #217.
However, it needs more test cases for merge.