setNumLines should take a number

[ronyeh]

vexflow/src/stave.ts

Lines 187 to 192 in 2315e7a

	getNumLines(): number {
	return this.options.num_lines;
	}
	setNumLines(lines: string): this {
	this.options.num_lines = parseInt(lines, 10);

the line with the parseInt has been in the VexFlow codebase for 5 years.

However, the only tests case that touches the setNumLines() is in tabstave_tests.js, and it passes in a number.

Can I just change the param's type to number? The : string type was added during migration, probably because Rodrigo saw the parseInt().

[mscuthbert]

yes, please!

btw -- it'd be very good to eventually expose options.lineConfig since at least at last I looked there was no public interface. I use it to set 1-line staves to show the middle line and not the bottom line as it defaults to.

[rvilarl]

#861 something similar was discussed in regards getValueForString see the resolved conversations.
I agree that it makes more sense to use number. And I also definetively forgot to make the approach in #861 wich would have been.

 setNumLines(lines: string | number): this { 
   this.options.num_lines = Number(lines); 

[ronyeh]

So why was the method signature assuming string? I see your comments: *The group defining @type/vexflow assumed string, wich is wrong, but I based my initial decision on their guess. However all the instances in this library use numbers, this is why I had to do string | number. My failure.* *Once this is ported, @types/vexflow is no longer needed and I believe that leaving a dual type due to the failure in @types/vexflow and my falure making it a dual type is not right.* If true, we should just set it to number. That's the only type that makes sense really. If someone out there actually passed a string to setNumLines, I'm okay with them having to change the code to fix this "breaking change."

…

On Thu, Jul 29, 2021, 8:39 PM Rodrigo Vilar ***@***.***> wrote: #861 <#861> something similar was discussed in regards getValueForString see the resolved conversations. I agree that it makes more sense to use number. And I also definetively forgot to make the approach in #861 <#861> wich would have been. setNumLines(lines: string | number): this { this.options.num_lines = Number(lines); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1083 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB2MCKNHEQXOIDEOXL4P6LT2INH3ANCNFSM5BHQRCOA> .

[rvilarl]

I wanted to leave number only, but Mohit prefered to include string should any library user use it so as not to change the interface

[ronyeh]

Is there any evidence that people have passed strings to a method called .setNumLines()?

It really does look buggy that getNumLines() returns a number but setNumLines(lines: string) takes a string. The TypeScript compiler is definitely not happy about that. :-)

My guess is that the original author of setNumLines() figured it would be extra robust to use parseInt(n, '10') because it works with both strings and numbers. Then whoever decided to write the original typescript definitions for DefinitelyTyped saw the source code and decided it should be a string, since the source said parseInt() and TypeScript declares an error if you pass a number to parseInt().

My vote would be to just change it to number.

When we make the next official release, we can bump the version number in a semantic versioning way which suggests breaking changes (e.g., VexFlow 4.0.0).

We can also list the known API changes (such as this one) which might break your existing deployment. That way we can fix errors in our API such as this one.

[0xfe]

Hi folks -- Yes, I suggested string so we don't break the interface, particularly during the migration.

Now that everything works, as Ron suggested, it's a good opportunity to rationalize the types and maintain a log of interface breaking changes, so we can bump the major version.

There's a lot of other calls in the library that do string | number and we can fix all of them now. So, let's do it. :-)

[ronyeh]

Sounds good. We can log breaking changes in a CHANGELOG.md.