FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow any module name by hdgarrood · Pull Request #182 · purescript/trypurescript · GitHub

Allow any module name - #182

Closed
hdgarrood wants to merge 2 commits into
purescript:masterfrom
hdgarrood:dont-check-module-name
Closed

Allow any module name#182
hdgarrood wants to merge 2 commits into
purescript:masterfrom
hdgarrood:dont-check-module-name

Conversation

Copy link
Copy Markdown
Collaborator

Seems to work. Fixes #174

Copy link
Copy Markdown
Collaborator Author

Ah, this behaves oddly if you give the module a name of an existing module in the package set. For example, if I call the current module Data.Semiring and try to use + within it, then I get a "ReferenceError: $foreign is not defined". Perhaps I should rewrite the module name to Main to ensure that it doesn't clash with any module name in the package set?

Copy link
Copy Markdown
Collaborator Author

This now seems to work even if I try to redefine an existing module. I haven't been able to think of a way that rewriting the module name like this would be an issue, it seems like a slightly nicer UX for what is essentially the same idea (only modules with a specific module name are allowed).

thomashoneyman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A nice improvement. We’ll have to revisit this if Try PureScript ever supports multiple files / modules but that’s a faraway future.

Comment thread server/Main.hs
-- Rewrite the module name to "$Main" in order to ensure that the
-- module name doesn't clash with any existing module names in
-- the package set.
let rewriteModuleName (P.Module ss coms _ decls refs) = P.Module ss coms (P.moduleNameFromString "$Main") decls refs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If someone attempts to play with module export syntax this might be cause problems with:

module Foo (module Bar, module Foo) where ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'll wager this is by far an edge case, however 😆

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, good point. I think re-exporting main from elsewhere seems plausible to want to do, so perhaps this isn't the best approach. Maybe we should assemble a set of used module names from the externs files, and then instead of rewriting, just check that the given module name is not included in that set?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow module names other than Main

3 participants


Back | FazBrowse Home | New Git URL