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

validators.py created #282

Closed

Conversation

lucas-lemos-da-silva
Copy link

@lucas-lemos-da-silva lucas-lemos-da-silva commented Dec 27, 2024

close #7 the module validators.py was created with the function isnumber. On the file loaddata.py the old "isfloat" was deleted. Please let me know if something is missing.

Copy link

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

@bobleesj
Copy link
Contributor

Great @lucas-lemos-da-silva

We need to have a news file - Could you try to add a news file and i think we have the instruction provided?

We also need to run pre-commit before making a PR, could you also do that, i think we also have that in the instruction.

@bobleesj
Copy link
Contributor

Also please refer to the "Lessons Learned" gitlab page on how to design commit msgs and PR titles

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Thanks for this @lucas-lemos-da-silva

I think we maybe don't want validtaors.py inside parsers do we? It is a more boadly useful capability.

Also, I am not sure that isnumber() is really a validator per se. maybe we want to put it in to tools.py as a tool? We can discuss. @bobleesj?

The function seems to be called isnumber() but it is actually testing for whether the object can be converted to a float, not a number.

I think we maybe want to start with some tests and then we can work on the function.

I expect that this change in the code API will break things elsewhere too, so I would expect that there are likely other files that need to be updated. @lucas-lemos-da-silva do tests pass locally after this change?

@lucas-lemos-da-silva
Copy link
Author

Thank you for the comments! The current isnumber() function checks if an object can be converted to a float (almost all numbers). Upon closer inspection, I realized that it excludes complex and imaginary numbers. I am now exploring alternatives. Regarding the local tests, they do not seem to be working. Apologies for that. I am working on identifying the issue.

@lucas-lemos-da-silva
Copy link
Author

I will close this PR because I did not sync with the main before creating a branch. I will continue our discussion in a new PR. @sbillinge I am in touch with @bobleesj right now.

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

Successfully merging this pull request may close these issues.

factor out isfloat to a separate file
3 participants