Skip to content

Replace confit with confuse - #2061

Closed
jackwilsdon wants to merge 1 commit into
beetbox:masterfrom
jackwilsdon:use-confuse
Closed

Replace confit with confuse#2061
jackwilsdon wants to merge 1 commit into
beetbox:masterfrom
jackwilsdon:use-confuse

Conversation

@jackwilsdon

@jackwilsdon jackwilsdon commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

Use sampsyo/confuse (previously named confit) throughout beets instead of having our own copy of it.

This is part of #1966 that has been separated at the request of @jrobeson. Go there if you would like to see our discussion so far.

Note: This PR should not be merged until we've notified packagers of the new dependency (i.e. the 1st of August and 1.4.0).

@jackwilsdon jackwilsdon mentioned this pull request Jun 20, 2016
3 tasks
@sampsyo

sampsyo commented Jun 20, 2016

Copy link
Copy Markdown
Member

Nice!! This is looking fantastic! Great work!

There's one last unresolved question in my mind: should we somehow provide an alias so import statements in old plugins still work? Or should we depend on their developers to import confuse directly?

@jackwilsdon

Copy link
Copy Markdown
Contributor Author

See I was thinking about that before but I'm not entirely sure, what do you think of making a confit alias but logging a deprecation message in verbose mode? We could make this more severe in the future (i.e. log in normal info mode).

@sampsyo

sampsyo commented Jun 20, 2016

Copy link
Copy Markdown
Member

Great idea! I'm not 100% sure how to accomplish such a warning, but I have every confidence that you do. 👍

@jackwilsdon

Copy link
Copy Markdown
Contributor Author

Well when you import a file the file is only executed once, meaning we could just get the beets logger (using logger.getLogger) and log a warning to that. It would only be printed once no matter how many times it is imported 😄.

@jackwilsdon

jackwilsdon commented Jun 20, 2016

Copy link
Copy Markdown
Contributor Author

Well I've found a rather interesting trick. Here is the contents of confit.py:

for key, value in __import__('confuse').__dict__.items():
    globals()[key] = value

del key, value

And the odd behaviour:

from beets.util import confit

print(confit.__file__) # .../confuse-0.4.0-py2.7.egg/confuse.py

I understand why it happens but I don't know if it is good or not, maybe we should ignore all variables prefixed with __ or even _?

@ghost

ghost commented Jun 20, 2016

Copy link
Copy Markdown

If we're really about to do this, then I'd like to get my changes merged into confit first. Also, what do folks think about making six a confit dependency?

@jackwilsdon

Copy link
Copy Markdown
Contributor Author

What features of six do we need in confuse? If you're looking to make changes you may be best making them both here and in sampsyo/confuse for now, as this isn't going to be merged for a while (at least the 1st of August).

@sampsyo

sampsyo commented Jun 20, 2016

Copy link
Copy Markdown
Member

Confuse is already Python 2/3 hybrid, FWIW.

@ghost

ghost commented Jun 20, 2016

Copy link
Copy Markdown

depends on how you feel about the ergonomics of some things. There are places in beets where we have to work around set bytes, and then don't decode them on exit when it seems like they should

@ghost

ghost commented Jun 20, 2016

Copy link
Copy Markdown

@jackwilsdon : why until august?

@jackwilsdon

jackwilsdon commented Jun 20, 2016

Copy link
Copy Markdown
Contributor Author

See the linked comment from @sampsyo in the OP 😄.

@ghost

ghost commented Jun 20, 2016

Copy link
Copy Markdown

i should learn to read. I didn't go that far up!

@ghost

ghost commented Jun 20, 2016

Copy link
Copy Markdown

@sampsyo : if that's the case, why did you tell me to announce six already, and that we could start using it once your bugfix release is over?

@sampsyo

sampsyo commented Jun 20, 2016

Copy link
Copy Markdown
Member

six is an easier dependency to add, since it's already packaged for all distributions. The other ones will require downstream people to introduce new packages for the code we're splitting off.

@arcresu

arcresu commented Apr 7, 2019

Copy link
Copy Markdown
Member

It looks like this branch has fallen behind. If I'm understanding correctly, the original hold up was about notifying distro packagers about the new dependency. From my experience with Debian packaging there's quite a bit of inertia to adding new packages in general given that they so often get neglected over time. "We need to package confuse to be able to update beets" makes for a stronger argument than "we should package confuse because beets is thinking about using it". I would propose that we just go ahead and introduce the dependency and at worst the distro packages would lag behind for a little bit. I'm not familiar with other distros though so things might work a bit differently to Debian's workflow.

@sampsyo

sampsyo commented Apr 7, 2019

Copy link
Copy Markdown
Member

In my fairly narrow experience, Debian represents the "high water mark" for difficulty in getting new packages and updates approved. (This is not meant as a criticism; it's just that Debian has settled on a careful, methodical, "high-touch" process that's at the opposite end of the spectrum from most other distros.) You're right that jumping right in and making it a dependency might be just what's needed to spur packaging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants