david-yz-liu / memory-viz

Javascript library for creating beginner-friendly memory model diagrams.
https://www.cs.toronto.edu/~david/memory-viz/demo/
2 stars 7 forks source link

Type annotations #41

Closed yoonieaj closed 4 months ago

yoonieaj commented 4 months ago

Proposed Changes

Added type interfaces and type annotations for functions and constants in style.ts. This should improve code organisation/readability. ...

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality) x
🚦 Test update (change that only adds or modifies tests)
📚 Documentation update (change that only updates documentation)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

After opening your pull request:

Questions and Comments

Hello Professor Liu! I have a few questions about the changes I've made so far:

  1. The code doesn't seem to care whether the DrawnObject attributes are optional, so I'm not sure if I made the right call on which attributes should be optional?
  2. Since the preset Styles only specify certain attributes, can I leave every one of these attributes as optional?
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9372440637

Details


Totals Coverage Status
Change from base Build 9327208107: 0.0%
Covered Lines: 380
Relevant Lines: 423

💛 - Coveralls
david-yz-liu commented 4 months ago
  1. Weird that the compiler doesn't seem to be catching that, but we can look into that later. I think show_indexes and style can both be optional as well.
  2. Yes it's good for all Style attributes to be optional 👍
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9389923649

Details


Totals Coverage Status
Change from base Build 9376313216: 0.0%
Covered Lines: 381
Relevant Lines: 424

💛 - Coveralls
yoonieaj commented 4 months ago

Proposed Changes

Added type interfaces and type annotations for functions and constants in style.ts. This should improve code organisation/readability. ...

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality) x
🚦 Test update (change that only adds or modifies tests)
📚 Documentation update (change that only updates documentation)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

After opening your pull request:

Questions and Comments

(Include any questions or comments you have regarding your changes.)

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9390090716

Details


Totals Coverage Status
Change from base Build 9376313216: 0.0%
Covered Lines: 381
Relevant Lines: 424

💛 - Coveralls
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9457996657

Details


Totals Coverage Status
Change from base Build 9410692102: 0.0%
Covered Lines: 388
Relevant Lines: 429

💛 - Coveralls
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9458015375

Details


Totals Coverage Status
Change from base Build 9410692102: 0.0%
Covered Lines: 388
Relevant Lines: 429

💛 - Coveralls
coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9474095102

Details


Totals Coverage Status
Change from base Build 9410692102: 0.0%
Covered Lines: 388
Relevant Lines: 429

💛 - Coveralls