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

MF info properties untyped #226

Closed
stropitek opened this issue Nov 28, 2024 · 8 comments
Closed

MF info properties untyped #226

stropitek opened this issue Nov 28, 2024 · 8 comments

Comments

@stropitek
Copy link

stropitek commented Nov 28, 2024

The 2 properties monoisotopicMass and observedMonoisotopicMass can exist on the PartInfo object but are not typed.

  const observedMass =
    // @ts-expect-error types are wrong in MF library
    (info.observedMonoisotopicMass || info.monoisotopicMass) as number;
@stropitek
Copy link
Author

They are untyped because there is an option to override the property name to which the result is assigned.

emFieldName = 'monoisotopicMass',
msemFieldName = 'observedMonoisotopicMass',

It is used by octochem.

https://github.com/search?q=org%3Acheminfo%20emFieldName&type=code

@lpatiny do you know the reason for this?

@lpatiny
Copy link
Member

lpatiny commented Dec 3, 2024

There is some history behind it and the fact that on one end I wanted to use 'short' properties names (em and mz) and on the other one a property name that means something (monoisotopicMass and observedMonoisotopicMass).

I think it will be time to make some big refactoring and breaking changes and only keep monoisotopicMass and observedMonoisotopicMass but I need to find the time to do it (a lot of implications).

@targos
Copy link
Member

targos commented Dec 3, 2024

In the mean time, what should we do? Make the return type dynamically depend on these options?

@stropitek
Copy link
Author

Make the return type dynamically depend on these options?

I can try to do that, but I'm not very used to be defining types in jsdoc. I don't even know if it's possible.

@targos
Copy link
Member

targos commented Dec 3, 2024

You can mix ts and js files in this project, and import from ts in the js doc if it easier for you

@lpatiny
Copy link
Member

lpatiny commented Dec 4, 2024

Could you just use the library with defining the type in your project for a while ? Because I would really prefer we make breaking changes rather than making complex types.

Same is true when fwhm = 0. We ned another function in this case so that the return type is constant.

You can open issues with all the inconsistencies in this library.

@stropitek
Copy link
Author

Could you just use the library with defining the type in your project for a while ?

We can ignore the type problem (it is what we do right now). But if we fix it, it's the same amount of work to fix it in the library so we'd better do that. If done with good type inference it would not force user to do something more complex at the typescript level when using the getInfo method.

@tpoisseau
Copy link
Collaborator

Closed by #241 (partial migration to typescript with advanced types)

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

No branches or pull requests

4 participants