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

Fixed closure import problem when defining a library #336 #337

Merged
merged 1 commit into from
Mar 19, 2016
Merged

Fixed closure import problem when defining a library #336 #337

merged 1 commit into from
Mar 19, 2016

Conversation

PhucVH888
Copy link
Contributor

We have fixed this problem by adding more cases to due with import in Utils.hs

extendAccumProgram f acc0 p@Program{functions, traits, classes, imports} =
  (acc4, p{functions = funs', traits = traits', classes = classes', imports = imports'})

      .......
      (acc4, imports') = List.mapAccumL (extendAccumImport f) acc3 imports
      extendAccumImport f acc i@(PulledImport{iprogram}) =
        (acc', i{iprogram = iprogram'})
        where
          (acc', iprogram') = extendAccumProgram f acc iprogram
      extendAccumImport _ _ _ = error "Util.hs: Non desugared imports during extendAccumProgram"

@EliasC
Copy link
Contributor

EliasC commented Feb 25, 2016

Minor comment: You don't need to write the diff in the description, since it's already visible under "Files changed". I'd rather see a more high level description of what was done ("Added more cases" doesn't tell me anything useful). No need to fix it for this PR though as we were all in the room when it was fixed :)

@EliasC
Copy link
Contributor

EliasC commented Feb 25, 2016

The big question is who will merge this, as most of us were in the room when it was written! 😮

@PhucVH888
Copy link
Contributor Author

:)), may be Tobias or Dave, or one of us? But anyway, do we need a testcase with the expected result ?

@EliasC
Copy link
Contributor

EliasC commented Feb 25, 2016

But anyway, do we need a testcase with the expected result ?
I think that's a good idea. You could possibly extend one of the module tests to import a closure.

@PhucVH888
Copy link
Contributor Author

Test case is added.

@EliasC
Copy link
Contributor

EliasC commented Feb 25, 2016

Great, is everyone fine with merging this?

@kikofernandez
Copy link
Contributor

yes!

@albertnetymk
Copy link
Contributor

It's best to squash the two commits into one so that you could learn how to use rebase.

@kikofernandez
Copy link
Contributor

+1

@kikofernandez
Copy link
Contributor

@PhucVH888 what's the status of squashing these two commits?

@PhucVH888
Copy link
Contributor Author

These commits have not been squashed yet. I think it would be nice if you or @albertnetymk can show me how to do this tomorrow.

@kikofernandez
Copy link
Contributor

I agree but first I would recommend that you read the official doc on rebase and, if you are still unsure, we solve your doubts. The important step is that you get a better understanding of the git jargon

@PhucVH888
Copy link
Contributor Author

I have squashed these two commits.

EliasC added a commit that referenced this pull request Mar 19, 2016
Fixed closure import problem when defining a library #336
@EliasC EliasC merged commit eac5f6d into parapluu:development Mar 19, 2016
@kikofernandez kikofernandez deleted the utils branch March 19, 2016 08:26
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.

4 participants