Skip to content

Conversation

ridge-kimani
Copy link
Contributor

Open in Gitpod know more

Describe your change:

  • Documentation change?
    Most docstrings use Integer in the maths package as the input params instead of Number. Number is the accepted param since it directly correlates with the datatype Number

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new JavaScript files are placed inside an existing directory.
  • All filenames should use the UpperCamelCase (PascalCase) style. There should be no spaces in filenames.
    Example:UserProfile.js is allowed but userprofile.js,Userprofile.js,user-Profile.js,userProfile.js are not
  • All new algorithms have a URL in their comments that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@ridge-kimani ridge-kimani changed the title chore: update maths variables in docstrings to be Number as opposed to Integer chore: update maths variables in docstrings from Integer to Number Oct 7, 2023
Copy link
Collaborator

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Many of these were overly specific previously and are in fact not limited to integers, but you also applied this change to many places where this isn't the case (ex. - not exhaustive: liouville function, hexagonal numbers, find max recursion left & right etc.)

@ridge-kimani
Copy link
Contributor Author

Most of those files have the { Integer } param which isn't a valid type. The acceptable valid type is { Number } and that's the purpose of the changes.

@appgurueu
Copy link
Collaborator

Nitpick: In the JSDoc examples I'm seeing number. Are you sure that Number is any more "valid" than Integer?

Integer isn't an "invalid" type per se. Custom types or class-defined types would be written like this. Integer is supposed to mean "a number, but whole and usually within some safe integer range", but we haven't defined that anywhere. Ideally files which use this "type" should contain a @typedef tag in the docs. We could also define types like this in one file and have other files require it, but this seems to be somewhat awkward.

I don't think just changing all occurrences of Integer to number is a proper solution since it removes valuable information.

@ridge-kimani
Copy link
Contributor Author

In the typedef it's either Number or number, it's not case-sensitive.

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.

3 participants