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

Type System Improvements #216

Merged
merged 17 commits into from
Mar 27, 2020
Merged

Type System Improvements #216

merged 17 commits into from
Mar 27, 2020

Conversation

gdotdesign
Copy link
Member

This PR is an RFC for visual improvements for the code itself.

I feel like that Mint code has some visual clutter which can be resolved quite easily, but for some we need type inference.

Removing some elements helps reduce visual complexity, makes it easier to read and reason about the code, also it has the additional benefit of writing less.

Types

For functions, states, properties and computed properties the type (or return type) is required, and for me sometimes it's a burden to write them. These type definitions can be made optional.

The only downside of that is that for recursive functions and for the component records we need to calculate static type signatures and without a defined type we can use a type variable a but having a type for helps with that.

Type definitions elsewhere will still be required (arguments, provider, records, etc...)

The fun keyword

It is possible to remove the fun keyword which in hindsight is not needed.


Here is an example of how a component file looks now and how it can look with this PR:

global component LoadingOverlay {
  state loading : Bool = false

  style base {
    cursor: progress;
    position: fixed;
    z-index: 10000;
    bottom: 0;
    right: 0;
    left: 0;
    top: 0;
  }

  fun show : Promise(Never, Void) {
    next { loading = true }
  }

  fun hide : Promise(Never, Void) {
    next { loading = false }
  }

  fun render : Html {
    if (loading) {
      <div::base/>
    } else {
      <></>
    }
  }
}
global component LoadingOverlay {
  state loading = false

  style base {
    cursor: progress;
    position: fixed;
    z-index: 10000;
    bottom: 0;
    right: 0;
    left: 0;
    top: 0;
  }

  show {
    next { loading = true }
  }

  hide {
    next { loading = false }
  }

  render {
    if (loading) {
      <div::base/>
    } else {
      <></>
    }
  }
}

This PR can be merged as-is, all the tests pass, however I still plan to work on this.

The fun keyword is optional but the compiler throws a warning that it will be removed in the future and the formatter automatically removes them.

Please give your feedback and if there is anything else which you think can be removed just let me know.

@gdotdesign gdotdesign changed the title 🚧 Code Visual Improvements 🚧 RFC: Code Visual Improvements Mar 14, 2020
@s0kil
Copy link
Contributor

s0kil commented Mar 14, 2020

The Promise(Never, Void) Type is very common, this PR will make it easier.

@Namek
Copy link
Contributor

Namek commented Mar 14, 2020

I would leave fun as it is. It's easier to search for a function definition when it is prefixed.

Other than that, I would like to have an ability to restrict/write types of variables in try block.

Oh, and I love that it's all optional. Especially anonymous functions (used with mapping, filtering) can benefit from that.

@gdotdesign
Copy link
Member Author

Other than that, I would like to have an ability to restrict/write types of variables in try block.

It's easy to add it as an optional thing, I can see that it could be helpful from some situations, I never needed myself it though.

I would leave fun as it is. It's easier to jump in view of I can search a function definition when it is prefixed.

I have mixed feelings here as well. GitHub need to implement some kind of polls for issues 😂

@Sija
Copy link
Member

Sija commented Mar 15, 2020

I would leave fun as it is. It's easier to jump in view of I can search a function definition when it is prefixed.

I have mixed feelings here as well. GitHub need to implement some kind of polls for issues 😂

@gdotdesign It's pretty easy!

Add a 👍 reaction to this comment for leaving fun or 👎 for removing it :)

@@ -6,7 +6,7 @@ store Test {
}

component A {
connect Test exposing{a,b,c,d}
connectTest exposing{a,b,c,d}
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesnt seem right

Copy link
Member Author

Choose a reason for hiding this comment

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

Strictly speaking the parser does not need white-space between the keyword and the name of the store here.

It's because there is no tokeniser phase, the parser directly produces the AST.

}

fun just (value : a) : Maybe(a) {
`new Just(value)`
``
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it empty now?

Copy link
Member Author

Choose a reason for hiding this comment

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

These tests does not execute JavaScript code, it just parses the code and type checks based on the annotations, so it does not make sense having that here.

@gdotdesign gdotdesign changed the title 🚧 RFC: Code Visual Improvements 🚧 Type System Improvements Mar 22, 2020
@gdotdesign
Copy link
Member Author

gdotdesign commented Mar 22, 2020

By popular demand the fun keyword will not be removed 🙂 and changes were reverted.

Now this PR contains the following:

  • optional return type definition for functions and inline functions
  • optional type definition for state, get and property
  • default value for properties can now be omitted Required properties #132
  • restricting type of arrays: [] of Number
  • restricting type of inline JavaScripts:
`"Hello"` as String

src/type_checker.cr Outdated Show resolved Hide resolved
@gdotdesign
Copy link
Member Author

Two more issues has been implemented here:

@gdotdesign gdotdesign merged commit c6a57d9 into master Mar 27, 2020
@gdotdesign gdotdesign deleted the type-inference branch March 27, 2020 15:28
@gdotdesign gdotdesign added this to the 0.9.0 milestone Mar 31, 2020
@gdotdesign gdotdesign changed the title 🚧 Type System Improvements Type System Improvements Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants