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

Fix #17219 Corrected the Fraction output logic #10

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

HardikGoyal2003
Copy link
Member

Overview

  1. This PR fixes or fixes part of #17219.
  2. This PR does the following: Updates the output for fractions in Guppy.

Essential Checklist

  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have followed the build process and "verifying your changes" section mentioned in the readme file.
  • I have followed the instructions for making a code change.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
  • The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

Proof that changes are correct

Screencast.from.02-03-24.12.21.14.AM.IST.webm

PR Pointers

@HardikGoyal2003
Copy link
Member Author

@seanlip PTAL! Thanks!

Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

@HardikGoyal2003 Which code am I actually supposed to review here? I don't see the source file(s) from which all these builds were generated, and I also don't understand why this has changed so much from the master branch. Could you please explain?

@HardikGoyal2003
Copy link
Member Author

@HardikGoyal2003 Which code am I actually supposed to review here? I don't see the source file(s) from which all these builds were generated, and I also don't understand why this has changed so much from the master branch. Could you please explain?

@seanlip I analysed and what I found is:

  • on running $ npm test it creates the hash changes in all files in coverage folder which is just to test and check test coverage.
  • Using build command doesn't make any changes in min.js file, hence we need to do it manually.
  • if we are changing the source files and keeping min.js file unchanged then no changes will be reflected on localhost, therefore localhost depends on min.js file.
  • if we change src file then run build command other files also gets updated but min.js file is constant.

Thanks!

@HardikGoyal2003
Copy link
Member Author

HardikGoyal2003 commented Mar 3, 2024

@seanlip PTAL! I have reverted the last commit and now test command has not been run but build command has been run and min.js has been edited manually. Thanks

@seanlip
Copy link
Member

seanlip commented Mar 4, 2024

@HardikGoyal2003 This is a concern IMO. We need a standard build process that also updates the min.js file, and this should be documented in the PR template etc. What should that process be? What does the guppy project itself usually use?

("Manual changes" isn't an acceptable answer.)

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.

2 participants