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

Allow main file to import data into mongo #21

Closed
wants to merge 34 commits into from

Conversation

smaysenhalder
Copy link
Collaborator

No description provided.

@audiodude audiodude changed the base branch from main to merge-netflix-to-wikidata October 11, 2024 04:33
@audiodude audiodude changed the base branch from merge-netflix-to-wikidata to merge-netflix-with-rate-limit October 11, 2024 04:33
@audiodude
Copy link
Collaborator

So I think this code looks pretty good, I haven't gotten a chance to review it in depth. However I think you're doing "too much" in one PR. Let's try to keep the PRs small and focused. There should've been:

  1. PR to put data/output directory in a constants file
  2. PR to escape quotes
  3. PR to include the Mongo code and even maybe a separate PR to integrate it into the main file.

I don't think it's worth it to break it up at this point, but just something to think about in the future. In general it should be "I have this task, so I'll create a branch/PR for it", not "I have this branch/PR, so whatever tasks I have will go there".

Thanks!

@audiodude audiodude force-pushed the merge_netflix_streamlining_function_calls branch from 162cf04 to 299243c Compare October 24, 2024 21:06
Copy link
Collaborator

@audiodude audiodude left a comment

Choose a reason for hiding this comment

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

Thanks for making all these changes. I hope I wasn't too harsh about the "one change per PR" thing, just trying to give guidance.

mediabridge/data_processing/wiki_to_netflix.py Outdated Show resolved Hide resolved
mediabridge/data_processing/wiki_to_netflix.py Outdated Show resolved Hide resolved
mediabridge/data_processing/wiki_to_netflix.py Outdated Show resolved Hide resolved
src/constants.py Outdated Show resolved Hide resolved
src/main.py Outdated Show resolved Hide resolved
src/main.py Outdated Show resolved Hide resolved
src/main.py Outdated Show resolved Hide resolved
Base automatically changed from merge-netflix-with-rate-limit to main October 26, 2024 01:04
Copy link
Collaborator

@jhanley634 jhanley634 left a comment

Choose a reason for hiding this comment

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

Please do not merge down the "from constants import *" line. It needs to mention specific symbols, not *, as an aid to the reader. We read code much more than we write it.

Copy link
Collaborator

@audiodude audiodude left a comment

Choose a reason for hiding this comment

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

Great job on these changes, we were able to resolve a lot of the conversations. Still a few things to do though.

main.py Outdated Show resolved Hide resolved
@jhanley634
Copy link
Collaborator

This is shaping up nicely, looks good.

from ... import * is not a Best Practice. Prefer to replace "*" with "insert_into_mongo, process_data". In general, you can usually delete the asterisk in your IDE and then ask it for help with autocomplete of specific symbols to import. Or go down to the call site, marked Red for Undefined, and right-click will help you adjust the import.

Copy link
Collaborator

@audiodude audiodude left a comment

Choose a reason for hiding this comment

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

Great work. Just a couple tiny things that could or could not be done in this PR. Ready to merge as-is.

src/main.py Outdated Show resolved Hide resolved
@jhanley634
Copy link
Collaborator

This branch is definitely moribund at this point. It has drifted away from main, and has merge conflicts in db/ and data_processing/ that I don't know how to properly resolve. Additionally the new .vscode/{extensions,settings}.json files would perhaps be better added to .gitignore. Consider abandoning and deleting this branch.

@audiodude
Copy link
Collaborator

Additionally the new .vscode/{extensions,settings}.json files would perhaps be better added to .gitignore

These have been intentionally checked into the repo in order to aid project members in setting up their IDEs correctly. They are the recommended settings for VSCode. As you said in another comment, your IDE is your power tool, and here we are "putting in the batteries" for you.

@audiodude audiodude mentioned this pull request Nov 24, 2024
@audiodude audiodude force-pushed the merge_netflix_streamlining_function_calls branch from 2870ce2 to 1d214de Compare December 13, 2024 03:18
@audiodude
Copy link
Collaborator

I tried rebasing this branch in order to merge it, but it's very out of date at this point.

The work to establish a constants.py has been sort of superseded by the work in #54.

We still need the Mongo code, but it should probably just be done in its own new PR at this point.

@audiodude audiodude closed this Dec 13, 2024
@audiodude audiodude deleted the merge_netflix_streamlining_function_calls branch December 13, 2024 03:23
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.

3 participants