New PSCi command: reload by hatashiro · Pull Request #2721 · purescript/purescript · GitHub

Regarding #2713

This PR update the list of commands in PSCi. The current :reset command is renamed to :clear, and a new command :reload is added. :reload is expected to work the same as :reload in GHCi.

Remove duplication beforing cons'ing a module.

The shortcut ':r' will be used for ':reload' later.

To avoid naming collision.

The :reload command will keep the current list of modules, just try to rebuild them.

What is this function for?

It's to prevent import duplication. Previously, when a module was imported twice, it was listed in the imported modules twice. Maybe the name renew is somewhat confusing?

Previously, when a module was imported twice, it was listed in the imported modules twice.

Is that necessary? Maybe that's what we need to fix?

Yes, that's what I fixed with the patch cd13ee4 included in this PR.

Before:

> import Data.Functor
> import Data.Functor
> import Data.Functor
> import Data.Functor
> :show import
import Data.Functor
import Data.Functor
import Data.Functor
import Data.Functor

>

After:

> import Data.Functor
> import Data.Functor
> import Data.Functor
> import Data.Functor
> :show import
import Data.Functor

>

I think we want to allow multiple imports with the same name though. Otherwise you can't do something like import something qualified and with selective imports at the same time.

I understand users can add duplicate imports and PSCi works without any problem. However, according to my understanding, duplicate imports mean nothing, but they are all listed in :show import. I still think we do not need to keep the identical imports in the import list of PSCi.

It's actually not about :r, but import X in PSCi. May I revert it from this PR and open a discussion?

However, according to my understanding, duplicate imports mean nothing

No, they are useful:

> import Prelude ((+))
> import Prelude as P

This means that I want to use + globally but qualify everything else. If the user typed that, we should preserve both.

Yes, but my update was removing only the identical ones, where identical means identical at everything including qualification, listed imports, listed hidings.

> import Prelude ((+))
> import Prelude as P
> :show import
import Prelude ((+))
import Prelude as P

> import Prelude
> import Prelude
> import Prelude ((+))
> import Prelude as P
> :show import
import Prelude
import Prelude ((+))
import Prelude as P

I think if we were going to do that, we should consolidate any imports under the same namespace. So for example

import Prelude ((+))
import Prelude (bind)
import Prelude as P ((*))
import Prelude as P (show)

should give

> :show import
import Prelude ((+), (*))
import Prelude as P (bind, show)

Without that, I'm not sure it's worth it to be honest. Also, we could do this once when the user types :show import, rather than doing it on every import.

Your suggestion looks better. I may upload an issue for the consolidation, and revert the renew from PR. Thanks for your review 👍

Looks great! Could you please add a test to tests/TestPsci.hs as well?

Will add a test.

Currently :clear and :reload are tested.

Even though module names are the same, an import can be allowed if they have different declaration types or qualifications.

Updated the renew to check not only the name, but the ImportedModule tuple itself. By this change, multiple imports with different forms work as expected:

> import Prelude
> import Prelude ((+))
> import Prelude
> import Prelude
> import Prelude as P
> import Prelude
> import Prelude ((+))
> import Prelude
> :show import
import Prelude
import Prelude ((+))
import Prelude as P

>

There have been some errors about compiling Eq1 and Ord1 but they are fixed by bower cache clean.

Moved to #2722.

Reverted duplication removal for PSCi imports.

Thanks!

Hyunje Jun added 4 commits March 8, 2017 18:46

Prevent imported modules from being duplicated

cd13ee4

Remove duplication beforing cons'ing a module.

Rename PSCi :reset to :clear

ee5caad

The shortcut ':r' will be used for ':reload' later.

Rename Reload browser command to Refresh

e48d931

To avoid naming collision.

Implement :reload command for PSCi

5fb8756

The :reload command will keep the current list of modules, just try to rebuild them.

paf31 reviewed Mar 8, 2017

View reviewed changes

Hyunje Jun added 2 commits March 9, 2017 12:37

Add test suites to test PSCi commands

4020c4d

Currently :clear and :reload are tested.

Allow PSCi import duplication in other forms

e226378

Even though module names are the same, an import can be allowed if they have different declaration types or qualifications.

Better error message for PSCi test suites

28a1153

hatashiro mentioned this pull request Mar 9, 2017

Consolidate import list in PSCi #2722

Open

Revert 'removing import duplication in PSCi'

a37816f

Moved to #2722.

paf31 approved these changes Mar 9, 2017

View reviewed changes

paf31 merged commit 52668d8 into purescript:master Mar 9, 2017

hatashiro deleted the repl-module-import branch March 10, 2017 01:05

hatashiro mentioned this pull request Mar 10, 2017

Test for PSCi evaluation. #2664

Closed

                    Left errs -> printErrors errs
                    Right _  -> put st
+                 where
+                 renew :: ImportedModule -> [ImportedModule] -> [ImportedModule]

New PSCi command: reload by hatashiro · Pull Request #2721 · purescript/purescript · GitHub

Uh oh!

New PSCi command: reload - #2721

New PSCi command: reload#2721paf31 merged 8 commits intopurescript:masterpurescript/purescript:masterfrom hatashiro:repl-module-importCopy head branch name to clipboard

Conversation

hatashiro commented Mar 8, 2017

Uh oh!

paf31 Mar 8, 2017

Choose a reason for hiding this comment

Uh oh!

hatashiro Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

paf31 Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

hatashiro Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

paf31 Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

hatashiro Mar 9, 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!

paf31 Mar 9, 2017

Choose a reason for hiding this comment

Uh oh!

hatashiro Mar 9, 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!

paf31 Mar 9, 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!

hatashiro Mar 9, 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!

paf31 commented Mar 8, 2017

Uh oh!

hatashiro commented Mar 9, 2017

Uh oh!

hatashiro commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hatashiro commented Mar 9, 2017

Uh oh!

hatashiro commented Mar 9, 2017

Uh oh!

paf31 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

New PSCi command: reload#2721
paf31 merged 8 commits into
purescript:masterfrom
hatashiro:repl-module-import

hatashiro Mar 9, 2017 •
edited
Loading

hatashiro Mar 9, 2017 •
edited
Loading

paf31 Mar 9, 2017 •
edited
Loading

hatashiro Mar 9, 2017 •
edited
Loading

hatashiro commented Mar 9, 2017 •
edited
Loading