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

Updated format for fraction output #7

Closed

Conversation

HardikGoyal2003
Copy link
Member

@HardikGoyal2003 HardikGoyal2003 commented Feb 20, 2024

Overview

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

What was wrong before

min.js file was formatted which was not needed, now to minify I used an online minify tool which wasn't that efficient so it also removed the important lines.

Solution

After knowing that it was breaking the code, I reverted all the formatting and changes done in that file, now just changed the required part with mantaining its initial formatting so no need to minify that again.

Proof that changes are correct

Build on local is working correctly.

scrnli_19_02_2024_15-37-03.mp4

@HardikGoyal2003
Copy link
Member Author

@seanlip PTAL! In the last PR min.js file was broken due to some external code. Now this has been updated and only certain part has been changed and no formating has been touched. Thanks!

@seanlip
Copy link
Member

seanlip commented Feb 22, 2024

Please provide video proof of correctness in this PR, update the title and description to give sufficient detail, and explain the steps/commands you used to minify the file (and what was wrong before). Thanks.

(This context is important for reviewers. Please make sure other PRs are similarly clear, otherwise reviewers will just return the PR to you and ask for more details, which isn't a great use of time.)

@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.

Still not reviewing this. Check your PR for correctness from a reviewer's perspective and make sure that all parts of it are correct and that all my points in the previous comment were fully addressed.

@HardikGoyal2003
Copy link
Member Author

HardikGoyal2003 commented Feb 22, 2024

Still not reviewing this. Check your PR for correctness from a reviewer's perspective and make sure that all parts of it are correct and that all my points in the previous comment were fully addressed.

@seanlip Follow up on this comment, I ran the commands for both the currently active version and the local version that has PR changes, to check differences in both version, output for both are:

Current active version

image
image

Local version after my changes

image
image

Analysis

As the output for both shows, there is no change in the min.js file but the other files have a lot of changes, should we add these changes or revert these changes?

@HardikGoyal2003 HardikGoyal2003 changed the title min.js updated Updated format for fraction output Feb 22, 2024
@seanlip
Copy link
Member

seanlip commented Feb 24, 2024

Thanks @HardikGoyal2003. Please add these changes and include an update to the README or the PR template to run this command and include the changes in every PR.

@HardikGoyal2003
Copy link
Member Author

@seanlip If I'm getting this correct, you are talking about the Guppy ReadMe file, and this PR's template as if we don't have any PR template for the Guppy repo??

@seanlip
Copy link
Member

seanlip commented Feb 24, 2024

The first is correct, and for the second I am suggesting we add a Guppy repo PR template (the same way as we do in Oppia) with this information.

For that PR template you can adapt the general one from Oppia as well.

@HardikGoyal2003
Copy link
Member Author

HardikGoyal2003 commented Feb 24, 2024

@seanlip I was editing the Readme file, and I thought to take a look at other files, this is how they look after the build,
so I feel adding build changes will not be a good idea.

Note : Before running those commands there were no errors.

Screencast.from.24-02-24.11.27.30.PM.IST.webm

@seanlip
Copy link
Member

seanlip commented Feb 24, 2024

Thanks. Before the build, were there errors? If not, what changed from before the build to after?

Wondering if the issue is with the build or with our past changes.

@HardikGoyal2003
Copy link
Member Author

@seanlip There were no errors before the build, as I have shown below
Screencast from 25-02-24 12:00:46 AM IST.webm

There are many changes including function changes, module changes, and many more changes I can't understand.

@seanlip
Copy link
Member

seanlip commented Feb 25, 2024

Ah, sorry, you did say that above. Thanks for checking.

Can you checkout commit 1759992 which is where we originally forked the repo, and then try building that? If it works fine then the problem is something we did along the way, so maybe checkout the next few commits here and see where the errors first occur. At that point we can decide whether to fix them or to file an issue and leave it be for now, but let's do a little more investigation to find the root cause of this since we ought to standardize our build instructions. Thanks!

@HardikGoyal2003
Copy link
Member Author

HardikGoyal2003 commented Feb 25, 2024

@seanlip I tried building using the commit you mentioned and it was also showing errors, then I analyzed more, and it was all Eslint errors which was highlighted by an extension. So we can add changes but I feel we should do this in 2 PRs,

  1. Changes I made
  2. Build changes

so that we will be assured whether the build is breaking the codebase or not because I'm pretty sure this time that my changes can't break. Thanks!

@seanlip
Copy link
Member

seanlip commented Feb 26, 2024

Yes, seems reasonable. Can you do another PR fixing the build changes first (just on the master branch), then we can merge that, and then apply the build process to this PR and all other PRs moving forward? Thanks.

@HardikGoyal2003 HardikGoyal2003 mentioned this pull request Feb 26, 2024
@HardikGoyal2003
Copy link
Member Author

HardikGoyal2003 commented Feb 26, 2024

@seanlip I have opened a PR with build file changes on the master branch, let's see after merging this PR. Thanks!

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