Adding build files #9

HardikGoyal2003 · 2024-02-26T20:53:05Z

Overview

Comment Reference
This PR does the following: Adds the build files

Video Proof of change:

Screencast.from.27-02-24.09.08.16.PM.IST.webm

HardikGoyal2003 · 2024-02-26T20:53:50Z

@seanlip PTAL! Thanks!

seanlip

@HardikGoyal2003 Please add proof of changes (i.e. that guppy still works properly).

Also were you planning to update/add the PR template with the checkbox + instructions to do a build, so that all future PRs to guppy will do that automatically? Would be good to get that process in place now so that the next PR can be a test of that workflow. (Here's the oppia/oppia version: https://github.com/oppia/oppia/blob/develop/.github/PULL_REQUEST_TEMPLATE.md)

HardikGoyal2003 · 2024-02-27T15:43:02Z

@seanlip PTAL! I remember that I needed to add a PR template and add build steps in the readme file. I am currently awaiting the merge of this PR to ensure that we do not encounter any issues with the build process after merging. Thanks!

seanlip · 2024-02-27T15:47:49Z

@HardikGoyal2003 I'm not sure why you're testing with guppy, you should be testing with the oppia app. Remember how last time we merged this and then your changes broke the oppia app, we shouldn't do that again.

Also, I think it's fine to add the template and build steps here since they have no effect on the correctness of the PR.

HardikGoyal2003 · 2024-02-27T18:04:53Z

@seanlip Okay, I will template and build steps with this PR! Just not sure how to test changes on the oppia app since it takes guppy from the online repo. I found that changing the node_modules/guppy-dev with new build files might work. Am I on the right track? Please let me know if this is the correct approach. If so, the oppia app is working fine with the build changes shown below. Thanks!

Screencast.from.27-02-24.11.28.16.PM.IST.webm

seanlip · 2024-02-27T18:07:41Z

No, you probably should update package.json temporarily and do a reinstall. Don't modify node_modules/ directly.

HardikGoyal2003 · 2024-02-27T19:10:20Z

No, you probably should update package.json temporarily and do a reinstall. Don't modify node_modules/ directly.

@seanlip It's working fine! I changed the guppy dependency path to a local path and restarted the server, it automatically updated the node_modules. Thanks!!

Package.json

"guppy-dev": "file:/home/hardik/oppia-guppy/guppy-dev",

HardikGoyal2003 · 2024-02-27T20:11:03Z

@seanlip PTAL! Thanks!

seanlip · 2024-02-28T18:00:36Z

@HardikGoyal2003 Please show video proof of changes that also shows what package.json looks like, thanks. (Reviewers need to verify this.)

HardikGoyal2003 · 2024-02-28T18:12:57Z

@seanlip Here is the video proof, Thanks!

Screencast.from.28-02-24.11.40.52.PM.IST.webm

seanlip

Thanks @HardikGoyal2003. Looks fine, left a few comments about the docs, thanks!

seanlip · 2024-02-28T18:57:45Z

PULL_REQUEST_TEMPLATE.md

+needed because {{Reason}}" and remove all the sections below.
+-->
+
+#### Proof of changes on desktop with slow/throttled network


You can drop this, and the mobile phone section.

seanlip · 2024-02-28T18:58:08Z

PULL_REQUEST_TEMPLATE.md

+## Proof that changes are correct
+
+<!--
+Add videos/screenshots of the user-facing interface to demonstrate that the changes


Can you update these instructions to be more specific to Guppy? Remove any that don't apply, and explain how to show proof of changes.

@seanlip I have already updated the PR template specific to guppy.

seanlip · 2024-02-28T18:58:32Z

README.md

@@ -50,6 +50,30 @@ Yes!  See the [contributors page](https://daniel3735928559.github.io/guppy/site/
 [roadmap](https://daniel3735928559.github.io/guppy/site/doc/roadmap.html)
 for an idea of where the project is heading.

+## Build process
+After making the changes in the Oppia's guppy fork, follow the below steps:
+* To get started, run and make sure you are in the guppy directory.


To get started, make sure you are in the guppy directory, then run the following:

seanlip · 2024-02-28T18:58:49Z

README.md

+## Error check
+After adding build changes on the local guppy directory, follow the below steps to ensure the changes made are correct.
+* Go to ``oppia/package.json`` file on local system.
+* Find the ``guppy-dev`` now change the path with your local guppy fork ([Example](https://github.com/oppia/guppy/pull/9#issuecomment-1967417760)).


Find the line with guppy-dev, and change the path to your ...

seanlip · 2024-02-28T18:59:15Z

README.md

+After adding build changes on the local guppy directory, follow the below steps to ensure the changes made are correct.
+* Go to ``oppia/package.json`` file on local system.
+* Find the ``guppy-dev`` now change the path with your local guppy fork ([Example](https://github.com/oppia/guppy/pull/9#issuecomment-1967417760)).
+* Now, restart the sever and manually test the guppy supported interaction thoroughly that the changes you made are not breaking any part of the code.


...thoroughly to ensure that ...

You might want to add something along the lines of "attach the video proof above"

HardikGoyal2003 · 2024-02-28T20:06:36Z

@seanlip PTAL! Thanks!

HardikGoyal2003 · 2024-02-28T20:17:17Z

@seanlip The force push was unintended; it occurred as a result of rapid action, likely due to a mistake or oversight rather than a deliberate choice. From next time onwards I will be more careful.

seanlip

@HardikGoyal2003 Looks good. Just trivial things.

Also I think you missed responding to this. Reminder to respond to all comments (please double-check that you've done so before asking reviewers for a follow-up review, thanks).

seanlip · 2024-02-29T17:38:58Z

README.md

+After adding build changes on the local guppy directory, follow the below steps to ensure the changes made are correct.
+* Go to ``oppia/package.json`` file on local system.
+* Find the line with ``guppy-dev``, and change the path to your local guppy fork ([Example](https://github.com/oppia/guppy/pull/9#issuecomment-1967417760)).
+* Now, restart the sever and manually test the guppy supported interaction thoroughly to ensure that the changes you made are not breaking any part of the code.


sever --> server

seanlip · 2024-02-29T17:39:14Z

README.md

+   ```
+   $ npm test
+   ```
+## Error check


Error check --> Verifying your changes

seanlip · 2024-02-29T17:39:39Z

README.md

+After adding build changes on the local guppy directory, follow the below steps to ensure the changes made are correct.
+* Go to ``oppia/package.json`` file on local system.
+* Find the line with ``guppy-dev``, and change the path to your local guppy fork ([Example](https://github.com/oppia/guppy/pull/9#issuecomment-1967417760)).
+* Now, restart the sever and manually test the guppy supported interaction thoroughly to ensure that the changes you made are not breaking any part of the code.


Did you see my earlier comment? "You might want to add something along the lines of "attach the video proof above"

@seanlip If I'm getting this correct, you have asked to add a Guppy-specific comment in the PR template under the video proof section. Please let me know?

Oh wait, sorry, scratch that -- this is the README file, not the PR template.

I think no changes are needed here, sorry about that. Thanks for clarifying.

seanlip · 2024-02-29T17:52:25Z

Also I think you missed responding to #9 (comment). Reminder to respond to all comments (please double-check that you've done so before asking reviewers for a follow-up review, thanks).

Sorry, looks like you did respond yesterday ... I'm not sure why it wasn't showing up for me when reviewing. In any case, thanks for the reply!

HardikGoyal2003 · 2024-02-29T17:57:44Z

@seanlip No issues, please can you confirm comment. Thanks!

HardikGoyal2003 · 2024-03-01T08:41:08Z

@seanlip PTAL! Thanks!

seanlip · 2024-03-01T17:30:28Z

PULL_REQUEST_TEMPLATE.md

@@ -13,7 +13,7 @@ If there is no corresponding issue number, fill in N/A where it says [fill_in_nu
 ## 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 error check mentioned in the [readme file](https://github.com/oppia/guppy/blob/master/README.md).
+- [ ] I have followed the build process and verifying your changes mentioned in the [readme file](https://github.com/oppia/guppy/blob/master/README.md).


I have followed the build process and "verifying your changes" section mentioned in the ...

HardikGoyal2003 · 2024-03-01T17:34:05Z

@seanlip PTAL! Thanks!

seanlip

Thanks! LGTM.

seanlip · 2024-03-01T17:47:31Z

(Btw I suggest next time that you don't develop from the master branch -- make a feature branch the usual way please. Thanks!)

HardikGoyal2003 · 2024-03-01T17:54:17Z

@seanlip I will remember that, but it was asked by you to make PR on the master branch in this comment. Now should I add build changes in this PR ??

Update: I have checked the dev server it's working fine after the build changes. Thanks!

seanlip · 2024-03-01T17:59:45Z

@seanlip I will remember that, but it was asked by you to make PR on the master branch in this comment. Now should I add build changes in this PR ??

When I say "make a PR to the master branch", I meant "make a PR that gets merged to guppy:master", not make a PR on the master branch of your own fork.

Thanks for confirming that things are working fine after the changes.

HardikGoyal2003 · 2024-03-01T18:06:34Z

Oh, okay, sorry for the misunderstanding.

Added build files

e3af7ec

HardikGoyal2003 mentioned this pull request Feb 26, 2024

Updated format for fraction output #7

Closed

seanlip requested changes Feb 27, 2024

View reviewed changes

seanlip assigned seanlip and HardikGoyal2003 and unassigned seanlip Feb 27, 2024

Added PR template and updated build instruction

b676ebf

seanlip requested changes Feb 28, 2024

View reviewed changes

HardikGoyal2003 added 2 commits February 29, 2024 01:09

Updating the PR template

38a7757

Updating the PR template

322e192

HardikGoyal2003 force-pushed the master branch from c70c33d to 322e192 Compare February 28, 2024 20:05

seanlip requested changes Feb 29, 2024

View reviewed changes

Typo correction

8d2e8a6

seanlip requested changes Mar 1, 2024

View reviewed changes

Correction in PR template

7a25698

seanlip approved these changes Mar 1, 2024

View reviewed changes

seanlip merged commit fbd9f5d into oppia:master Mar 1, 2024

Adding build files #9

Adding build files #9

Conversation

HardikGoyal2003 commented Feb 26, 2024 • edited

Overview

Video Proof of change:

HardikGoyal2003 commented Feb 26, 2024

seanlip left a comment • edited

Choose a reason for hiding this comment

HardikGoyal2003 commented Feb 27, 2024

seanlip commented Feb 27, 2024

HardikGoyal2003 commented Feb 27, 2024

seanlip commented Feb 27, 2024

HardikGoyal2003 commented Feb 27, 2024

HardikGoyal2003 commented Feb 27, 2024

seanlip commented Feb 28, 2024 • edited

HardikGoyal2003 commented Feb 28, 2024

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HardikGoyal2003 Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HardikGoyal2003 commented Feb 28, 2024

HardikGoyal2003 commented Feb 28, 2024 • edited

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Feb 29, 2024

HardikGoyal2003 commented Feb 29, 2024 • edited

HardikGoyal2003 commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HardikGoyal2003 commented Mar 1, 2024

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Mar 1, 2024

HardikGoyal2003 commented Mar 1, 2024 • edited

seanlip commented Mar 1, 2024

HardikGoyal2003 commented Mar 1, 2024

HardikGoyal2003 commented Feb 26, 2024 •

edited

seanlip left a comment •

edited

seanlip commented Feb 28, 2024 •

edited

HardikGoyal2003 Feb 29, 2024 •

edited

HardikGoyal2003 commented Feb 28, 2024 •

edited

HardikGoyal2003 commented Feb 29, 2024 •

edited

HardikGoyal2003 commented Mar 1, 2024 •

edited