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.

Sign up for GitHub

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

Jump to bottom

fix:fit binary-exponent.cpp to guidelines #2463

Closed
wants to merge 16 commits into from
Closed

fix:fit binary-exponent.cpp to guidelines #2463

wants to merge 16 commits into from

Conversation

ewd00010
Copy link
Contributor

Description of Change

FIX: added checking for negative exponents to both functions. Removed user interaction and replaced with tests function.

CHORE: added author, reformatted function documentation, added descriptions of what is happening in each algo and adhered to other repo standards

Checklist

  • Added description of change
  • Added file name matches File name guidelines
  • Added tests and example, test must pass
  • Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • I acknowledge that all my contributions will be made under the project's license.

Notes: Should we keep formatting text like \f$O(\log(n))\f$ in the comments or should they be removed?
Additionally i cant seem to get clang-tidy to work on my system, this code will need a once over with it

ewd00010 added 2 commits May 16, 2023 18:44
@ewd00010
Quality of life Update
b837b84 
FIX: added checking for negative exponents to both functions. Removed user interaction and repalced with tests function.

CHORE: added author, reformatted function documentation, added descriptions of what is happening in each algo and adhered to other repo standards
@ewd00010
Update binary_exponent.cpp
033b885
realstealthninja
realstealthninja requested changes May 23, 2023
View reviewed changes
math/binary_exponent.cpp Outdated Show resolved Hide resolved
@realstealthninja
Copy link
Collaborator

We use \f$ to render latex math equations they are necessary and imo should be required as they make the equations professional and presentable for more details check out doxygen's documentation on latex equations

ewd00010 and others added 2 commits May 23, 2023 17:44
@ewd00010 @realstealthninja
Update math/binary_exponent.cpp
8198035 
Co-authored-by: realstealthninja <[email protected]>
@ewd00010 @realstealthninja
Update math/binary_exponent.cpp
2e3313a 
Co-authored-by: realstealthninja <[email protected]>
@realstealthninja
Copy link
Collaborator

part of #2456

realstealthninja
realstealthninja previously approved these changes May 27, 2023
View reviewed changes
Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

Looks good! thank you so much for your contributions ❤️

@Panquesito7 Panquesito7 added enhancement New feature or request automated tests are failing Do not merge until tests pass labels May 31, 2023
Panquesito7
Panquesito7 requested changes May 31, 2023
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

For more details, you can check the clang-tidy warnings.
If you need any help fixing those, let us know. 🙂

math/binary_exponent.cpp Outdated Show resolved Hide resolved
math/binary_exponent.cpp Outdated Show resolved Hide resolved
math/binary_exponent.cpp Outdated Show resolved Hide resolved
math/binary_exponent.cpp Outdated Show resolved Hide resolved
math/binary_exponent.cpp Outdated Show resolved Hide resolved
ewd00010 and others added 4 commits June 3, 2023 14:24
@ewd00010 @Panquesito7
Update math/binary_exponent.cpp
e99f5e5 
Co-authored-by: David Leal <[email protected]>
@ewd00010 @Panquesito7
Update math/binary_exponent.cpp
bcc9d7d 
Co-authored-by: David Leal <[email protected]>
@ewd00010 @Panquesito7
Update math/binary_exponent.cpp
2602835 
Co-authored-by: David Leal <[email protected]>
@ewd00010
change long long to (u)int64_t
08d9554 
also one or two small wording changes
@ewd00010 ewd00010 requested a review from tjgurwara99 as a code owner June 3, 2023 14:30
tjgurwara99
tjgurwara99 reviewed Jun 5, 2023
View reviewed changes
math/binary_exponent.cpp Outdated Show resolved Hide resolved
math/binary_exponent.cpp Outdated Show resolved Hide resolved
math/binary_exponent.cpp Outdated Show resolved Hide resolved
ewd00010 added 3 commits June 5, 2023 20:06
@ewd00010
changed int64_t values to uint64_t
c1bea3c
@ewd00010
removed checking for < 0
4bae829
@ewd00010
multiple changes
324520e 
new assert test, new guard clauses ( now in both functions ), binary shift now in iterative function instead of /=2, base numbers can now be negative (exponents still unsigned if that's ok?)
@Panquesito7 Panquesito7 removed the automated tests are failing Do not merge until tests pass label Jun 8, 2023
ewd00010 and others added 2 commits June 10, 2023 14:56
@ewd00010 @Panquesito7
Update math/binary_exponent.cpp
c9ae29d 
Co-authored-by: David Leal <[email protected]>
@ewd00010
Update binary_exponent.cpp
7673a20 
small fix to assert test
Panquesito7
Panquesito7 approved these changes Jun 11, 2023
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks! 🚀

@Panquesito7 Panquesito7 requested a review from tjgurwara99 June 12, 2023 15:31
realstealthninja
realstealthninja approved these changes Jun 16, 2023
View reviewed changes
Copy link
Collaborator

@realstealthninja realstealthninja left a comment

Choose a reason for hiding this comment

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

LGTM!

@realstealthninja
Copy link
Collaborator

realstealthninja commented Jun 16, 2023 •
edited
Loading

just one conversation to resolve and you are good to go! @tjgurwara99

@Panquesito7
Copy link
Member

Will be merging in a few days if there are no objections. Ping me in case I forget.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Author has not responded to the comments for over 2 weeks label Aug 19, 2023
@realstealthninja
Copy link
Collaborator

@Panquesito7

@github-actions github-actions bot removed the stale Author has not responded to the comments for over 2 weeks label Aug 20, 2023
@github-actions
Copy link
Contributor

This pull request has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Author has not responded to the comments for over 2 weeks label Sep 19, 2023
@github-actions
Copy link
Contributor

Please ping one of the maintainers once you commit the changes requested or make improvements on the code. If this is not the case and you need some help, feel free to ask for help in our Gitter channel or our Discord server. Thank you for your contributions!

@github-actions github-actions bot closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@realstealthninja realstealthninja realstealthninja approved these changes

@Panquesito7 Panquesito7 Panquesito7 approved these changes

@alexpantyukhin alexpantyukhin alexpantyukhin approved these changes

@tjgurwara99 tjgurwara99 Awaiting requested review from tjgurwara99

Assignees
No one assigned
Labels
enhancement New feature or request stale Author has not responded to the comments for over 2 weeks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ewd00010 @realstealthninja @Panquesito7 @alexpantyukhin @tjgurwara99