casting to StemmableNote removed

[rvilarl]

Adding a few methods to Note it is possible to remove all the casting to StemmableNote in the source base. I find this generalisation very useful.

The method in Note are then overloaded in StemmableNote. The changes are not complex.

Several ts-nocheck resolved.

No Visual differences.

[rvilarl]

@ronyeh taking into consideration this changes, I believe that voice.getTickables() should return Note[] rather than Tickable[] wouldn´t you agree? Look at the ammount of as Note[] casting.

[0xfe]

Thanks for the change, Rodrigo. Can you fix the conflicts from the last merge please?

[rvilarl]

@0xfe done, ready to merge.

@0xfe @ronyeh I had to include the path in the import from util from RuntimeError in renderer_text. VSCode was considering a Node internal module. As a consequence I would prefer to add all the paths. Of course in another PR.

[ronyeh]

I'm OK with adding all the relative paths (e.g., ../src/). That will avoid any ambiguities like Element or Util. Feel free to do a search and replace and submit a PR on this. The only consideration is that VS code auto imports using the short form. We might end up with a mix of styles (which is what we had before I made all the tests consistent). I wonder if there is a setting that I'm missing that forced vs code to auto import with relative paths. I can do some research on this when I'm back with my computer.

…

On Sat, Oct 16, 2021, 11:46 AM Rodrigo Vilar ***@***.***> wrote: @0xfe <https://github.com/0xfe> done, ready to merge. @0xfe <https://github.com/0xfe> @ronyeh <https://github.com/ronyeh> I had to include the path in the import from util from RuntimeError in renderer_text. VSCode was considering a Node internal module. As a consequence I would prefer to add all the paths. Of course in another PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1182 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB2MCLMIR535PNRDW5WNNTUHHB67ANCNFSM5FT4ZXMA> .

[rvilarl]

@0xfe this one should be ready

[0xfe]

Thanks for the change Rodrigo, but digging in, it feels like we're losing more than we're getting. A lot of the useful type information captured in the code is now muddied because everything is being treated as a Note, which IMO is too crude of an abstraction.

This PR is titled "casting to StemmableNote removed", but it looks like we're removing all references to StemmableNote, regardless of utility.

For example, the following feels unnecessary:

- function applyStemDirection(group: StemmableNote[], direction: number) {
+ function applyStemDirection(group: Note[], direction: number) {

[0xfe]

I'm trying to understand the rationale here -- it feels like we're replacing compile-time checks with runtime checks. It's good to have StemmableNote a parameter here (you can't beam notes are not stemmable.)

[rvilarl]

We had no compile check Beam is being instantiated using as StemmableNote:

const notes = voice.getTickables() as StemmableNote[];
const beams = [
    f.Beam({ notes: notes.slice(0, 2), options: { autoStem: true } }),
    f.Beam({ notes: notes.slice(2, 4), options: { autoStem: true } }),
    f.Beam({ notes: notes.slice(4, 6), options: { autoStem: true } }),
    f.Beam({ notes: notes.slice(6, 8), options: { autoStem: true } }),
    f.Beam({ notes: notes.slice(8, 10), options: { autoStem: true } }),
    f.Beam({ notes: notes.slice(10, 12), options: { autoStem: true } }),
  ];

[rvilarl]

Thanks for the change Rodrigo, but digging in, it feels like we're losing more than we're getting. A lot of the useful type information captured in the code is now muddied because everything is being treated as a Note, which IMO is too crude of an abstraction.

The usage of as disables the compiler checks and It also does not provide a solution for runtime. Examples:
(note as StemmableNote).getStemX()
(note as StemmableNote).checkStem()
I believe that having the functions defined in Note causing a runtime error is more deterministic.

Please note that:

1. Note already had a abstractions for StemmableNote: hasStem()
2. We are adding only 5 abstractions: getStem(), getBeamCount(), setStemDirection(direction?: number), getStemX(), checkStem()

[rvilarl]

For example, the following feels unnecessary:

- function applyStemDirection(group: StemmableNote[], direction: number) {
+ function applyStemDirection(group: Note[], direction: number) {

Here there was also no compile check as the function was instantiated with:

Beam.generateBeams(voice.getTickables() as StemmableNote[], { groups, stem_direction });

which also uses as StemmableNote

[0xfe]

Thanks Rodrigo. In those cases, the as StemmableNote is better than accepting a Note because the programmer has to think about it -- they have to explicitly cast it to avoid a compile time failure. And, if they get it wrong they won't be silent failures, because they will fail at runtime anyway if those methods don't exist.

Re: hasStem -- I'd rather find a way to either remove it, or justify it as a matter of practicality. But adding 5 more stem abstractions simply because we have one doesn't feel like the right approach. I think StemmableNote is a good abstraction and we should keep it.

[rvilarl]

@0xfe This is ok.

The reason I had:

1. My understanding is that any and as are features to be avoided:
   https://medium.com/swlh/why-you-should-avoid-type-assertions-in-typescript-5494e3d04dd
2. I believe that a controlled fault throwing a RuntimeErroris better that a runtime failure.

@ronyeh do you have a suggestion on how to reduce the ammount of casting/type-assertions