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

Configurable #7

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Configurable #7

wants to merge 5 commits into from

Conversation

rockyroer
Copy link
Collaborator

@rockyroer rockyroer commented Jul 10, 2019

These updates allow the user to configure how the program works in settings.cfg:

  1. They can assign a maximum number of songs to download
  2. They can assign whether to add songs in ascending or descending order
  3. They can type a list of what charts to create

These updates also include some reorganization of the ChartData logic so that everything depends on just the chart_id.

- Added maximum number of songs key to settings-example.cfg
- Added playlist ordering key to settings-example.cfg
- Added helpful comments to settings-example.cfg
- Rewrote createbillboardplaylist.py to read in settings
- Did not implement those settings yet.
- Modified createbillboardplaylist.py to filter chart entries down to only include up to the maximum number of videos given in the config, and put them in either ascending or descending order
- Added Charts section and instructions for choosing charts in settings-example
- Changed createbillboardplaylist to read in charts_to_create as list
- Did not implement using this charts_to_create yet.
- Rewrote createbillboardplaylist to create only charts import from settings.cfg
- Hardcoded (incomplete) dictionary object to handle playlist metadata
- Extended ChartData object to have name, url, num_songs_phrase attributes assigned when chart is downloaded
- Modified some function declarations and calls to only depend on chart-id
- Moved most ChartData logic into BillBoard Adapter Class
@aag
Copy link
Owner

aag commented Jul 12, 2019

A few general notes:

  1. I've set up Travis CI in the project and it runs the linters and the tests on every commit. You can see the results of the runs here on this page. The checks are currently failing because there are some code formatting issues and the tests need to be updated. You can see the output of the checks by clicking the "Details" link next to each check. It's generally good practice to run all those commands locally before opening the PR, so you can fix any problems.

  2. It seems pretty limiting to hard-code the chart_info data in the script. The way it is now, someone could configure a chart that exists, but isn't in the chart_info object, and the script wouldn't work. I think it would be better to include the chart name in the config file, or change billboard.py so it provides the name.

  3. It seems to me like the max_number_of_songs and playlist_ordering settings should be chart-specific and not global to all charts. I might only want the top 10 songs from the R&B chart, but all of the Hot 100, but that wouldn't be possible with the current format.

Could you make a new commit that addresses those 3 things?

@rockyroer
Copy link
Collaborator Author

A few general notes:

  1. I've set up Travis CI in the project and it runs the linters and the tests on every commit. You can see the results of the runs here on this page. The checks are currently failing because there are some code formatting issues and the tests need to be updated. You can see the output of the checks by clicking the "Details" link next to each check. It's generally good practice to run all those commands locally before opening the PR, so you can fix any problems.

I don't know how to run these tests locally.

  1. It seems pretty limiting to hard-code the chart_info data in the script. The way it is now, someone could configure a chart that exists, but isn't in the chart_info object, and the script wouldn't work. I think it would be better to include the chart name in the config file, or change billboard.py so it provides the name.

I also don't like the hard-coded information. I was thinking of keeping the config file simple, with just list of chart-ids, and potentially putting the hardcoded data into a separate file -- like a csv file, that we could make for all possible charts. But that sounds like more work than its worth.
Honestly, I'd say the easiest thing to do would be to create a little simpler playlist description that is just the chart-id, number of songs, date and url, and not bother with a dictionary at all.

  1. It seems to me like the max_number_of_songs and playlist_ordering settings should be chart-specific and not global to all charts. I might only want the top 10 songs from the R&B chart, but all of the Hot 100, but that wouldn't be possible with the current format.

I also wondered about that. What might that look like in the config file?
'hot-100', 25, DESCENDING
'country', 10, ASCENDING
Can you have a multi-line key in a cfg file?

@aag
Copy link
Owner

aag commented Jul 12, 2019

I don't know how to run these tests locally.

Good point. I've just pushed an update to the README with the command you need to run.

I'd say the easiest thing to do would be to create a little simpler playlist description that is just the chart-id, number of songs, date and url, and not bother with a dictionary at all.

That would be easier, but it would make the playlist title pretty ugly. I'm working on a patch to billboard.py right now that will deliver the human-readable title for each playlist, so hopefully that will get merged and we can use that.

What might that look like in the config file?

With the current config file parser, we might have to do something like this:

chart_1_id = hot-100
chart_1_name = "Hot 100"
chart_1_max_songs = 10

chart_2_id = country-songs
chart_2_name = "Hot Country Songs"
chart_2_sort = ASCENDING

Then in the load_config() function we'd have to start from 1, count up, and try to form valid config keys until a chart_X_id is missing.

Alternatively, we could switch to the TOML library, which would allow us to do something like this:

[charts]
  [chart.1]
  id = "hot-100"
  name = "Hot 100"
  max_songs = 10

  [chart.2]
  id = "country-songs"
  name = "Hot Country Songs"
  sort = ASCENDING

@aag
Copy link
Owner

aag commented Jul 19, 2019

I'm working on a patch to billboard.py right now that will deliver the human-readable title for each playlist, so hopefully that will get merged and we can use that.

It took a little while, but my patch got accepted and a new version of billboard.py was released. I've updated the Pipfile in the master branch, so if you merge it into your branch and do pipenv install --dev you should get the newer version. Here's how you would do the merge:

$ git checkout master
$ git pull
$ git checkout configurable
$ git merge --no-ff master

These commands first pull down the newest version of the master branch, then bring in any new things in that branch into your feature branch.

If you have uncommitted changes in your local working copy, you'll have to run git stash before the above 4 commands, and git stash pop after them.

Once you do this, you can call .title on a ChartData object to get its human-readable title and we don't have to allow the user to configure it anymore.

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