AIToolsLab / questions

Writer-directed questions
0 stars 0 forks source link

React Migration #18

Closed RayHCai closed 1 year ago

nghtctrl commented 1 year ago

Looks good to me! This is something that I brought up with @kcarnold some time ago, but would we benefit from creating a separate repository named question-generation (or another appropriate name) instead of bothering about moving old things around into separate folder for each summer research sessions? This repository now looks like a small version of GitHub Teams rather than an actual repository.

nghtctrl commented 1 year ago

Looks good to me! This is something that I brought up with @kcarnold some time ago, but would we benefit from creating a separate repository named question-generation (or another appropriate name) instead of bothering about moving old things around into separate folder for each summer research sessions? This repository now looks like a small version of GitHub Teams rather than an actual repository.

This is not too important though. I realize that our GitHub Teams keeps three repositories, each signifying different desirable components of our writing system.

RayHCai commented 1 year ago

I think it would be beneficial if we moved the old research code to a different org or repo

nghtctrl commented 1 year ago

I think it would be beneficial if we moved the old research code to a different org or repo

Another small benefit is that, future researchers won't accidentally PR to high-level forks, which might cause unnecessary confusion.

kcarnold commented 1 year ago

I agree that the organization of this repo is not good, and that the fact that GitHub considers it a fork has been sometimes detrimental.

But I think it's helpful that we're in a single repo for frontend and backend, though, since many changes will affect both. And I think our custom editor frontend should probably be a peer to the Word addin code, reusing API where that makes sense.

So perhaps we go to:

- backend/
  - server.py
  - nlp.py
  - nlp_tests.py
- addin/
  - manifest.xml
  - package.json
  - src/
  - etc.
- custom_editor/ (or whatever we call it)
  - ...

but not in this PR. (Can someone move this to a separate Issue?)

And maybe in a newly created repo, so it doesn't show as a fork.

kcarnold commented 1 year ago

Now I'm looking at the code. Each PR should do one thing. Reorganizing the repo is a separate thing. Resist the urge to get it all done at once; that can slow down merging one part if there's discussion on another part.

kcarnold commented 1 year ago

I just saw that @SophiaSun18 was also requested for review. Her comments and visibility here would also be helpful. I'm going to merge just so we're not blocked, but Sophia you can still leave comments and questions here and the authors should deal with them (possibly in a future PR)

RayHCai commented 1 year ago

Should I create a separate PR to move the 2022 files? I think its a little distracting having them in the root directory

nghtctrl commented 1 year ago

@RayHCai I would suggest creating a new repo named reflections and migrating our code there, following this structure suggested by @kcarnold. Creating a new PR for that would be helpful.