fx: exclude fields typo

[whatphilipcodes]

Due to a typo the config for fields to exclude was ignored.

[coveralls]

Pull Request Test Coverage Report for Build 11106744800

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 91.123%

---

Totals
Change from base Build 10925838927:	0.0%
Covered Lines:	1018
Relevant Lines:	1114

---

💛 - Coveralls

[snejus]

I see the tests are failing because files in test/json/expected contain fields that are now excluded, so you may want to adjust them accordingly.

[whatphilipcodes]

I see the tests are failing because files in test/json/expected contain fields that are now excluded, so you may want to adjust them accordingly.

I can remove the comment entry for in all the jsons and that does fix the tests. However as I am unfamiliar with tests in general I am not sure if that wouldn't defeat the purpose a bit.

[snejus]

I see the tests are failing because files in test/json/expected contain fields that are now excluded, so you may want to adjust them accordingly.

I can remove the comment entry for in all the jsons and that does fix the tests. However as I am unfamiliar with tests in general I am not sure if that wouldn't defeat the purpose a bit.

Do not worry about writing any new tests - let's just fix the failing ones by adjusting the expected JSON data.

[snejus]

Actually, alternatively, it may be a better idea (and less work) to adjust the config that is used in tests to include comments as the additional field. Would you like to try this or shall I commit this myself?

I think this should be just a matter of removing "comments" from here

# tests/conftest.py
...
@pytest.fixture
def beets_config():
    return deepcopy({**DEFAULT_CONFIG, "exclude_extra_fields": ["comments"]})

effectively keeping just the deepcopy

@pytest.fixture
def beets_config():
    return deepcopy(DEFAULT_CONFIG)

[whatphilipcodes]

Yes you were right, keeping only the deepcopy worked perfectly! I updated the PR.
(Without requests the tests failed locally but I excluded it from this PR as it is included in #68)

[snejus]

Amazing stuff, thank you. It's weird that CI does not catch missing requests dependency - I'll need to look into this.

The only missing thing now is a note in CHANGELOG.md under the Unreleased section, something like

## Unreleased

### Fixed

- ...

[whatphilipcodes]

All done and ready to merge :)

[snejus]

Sorry - just one actually last thing - you want to rebase this off main as I merged a PR with a fix to the workflows file where I adjusted the changelog. Also see the comment regarding formatting.

[whatphilipcodes]

I might have butchered the rebase but after some struggeling I think it worked.

[snejus]

Somehow your changes from the other PR ended up here? 😅

[whatphilipcodes]

Oh yeah my bad, I rebased main in my fork to main in your repo and then rebased the PR branch on my main which already had both PRs merged. Lets see if I can fix that...

[whatphilipcodes]

Okay so I reverted the merge that incorporated the changes from my other PR and this should be correct now 😅
I apologize for the confusion I caused...

[snejus]

No worries! We do not want to update all of the dependencies here, since they will need a careful review, so it's best to do it separately.

To fix this, run:

git checkout main poetry.lock
poetry lock --no-update
git add -u
git commit -m 'Lock only beets dependency'

[snejus]

Perfect, we're good to go!