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

Add game lineups to dataset #231

Merged
merged 6 commits into from
Oct 10, 2023
Merged

Add game lineups to dataset #231

merged 6 commits into from
Oct 10, 2023

Conversation

LarchLiu
Copy link
Contributor

@LarchLiu LarchLiu commented Oct 8, 2023

@dcaribou
Is this approach reasonable? Are there any missing parts?

dcaribou/transfermarkt-scraper#72

@LarchLiu
Copy link
Contributor Author

LarchLiu commented Oct 8, 2023

Need to scrape lineups before running the prep process in github action (there is no game_lineups.json file in sources of dvc pull)

@dcaribou
Copy link
Owner

dcaribou commented Oct 8, 2023

Is this approach reasonable? Are there any missing parts?

Yes! It's looking great. I think you covered everything. I might add a minor suggestion or two, but you got it right 😄

Need to scrape lineups before running the prep process in github action (there is no game_lineups.json file in sources of dvc pull)

Yeah, the that will be required for the CI to pass. You'd also need to have write permissions in the dvc backend (s3), so you can run the dvc push command (unlike for dvc pull, pushing does require authentication). I need to prepare some instructions for this part. I can do the pushing of the data in your branch in the meantime.

@dcaribou
Copy link
Owner

dcaribou commented Oct 8, 2023

@LarchLiu, I've added the game_lineups to the raw data (see 1ad7e7a)

If you merge the latest from the master branch, the CI should be able to find the data now.

@LarchLiu LarchLiu marked this pull request as ready for review October 9, 2023 02:51
@dcaribou dcaribou merged commit 8fd956b into dcaribou:master Oct 10, 2023
2 checks passed
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