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

#fix 1060 Add 'con' to featuring artists markers #1143

Closed
wants to merge 5 commits into from

Conversation

Kraymer
Copy link
Contributor

@Kraymer Kraymer commented Dec 13, 2014

No description provided.

1, # Only split on the first "feat".
)
feat_tokens(extended=True).translate(None, '()'),
artist, 1, flags=re.IGNORECASE)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like Python 2.6 doesn't like the flags= keyword argument. 😢 I guess the way to work around this is to use re.compile.

@Kraymer
Copy link
Contributor Author

Kraymer commented Dec 14, 2014

Wouldn't it be useful to have a .py file somewhere to share beets-specific logic like that between plugins ?

@sampsyo
Copy link
Member

sampsyo commented Dec 14, 2014

Yes, perhaps it would—and that's more or less what we have already with the artresizer stuff.

In this case, though, the data is so simple that config options would almost be easier...

@Kraymer
Copy link
Contributor Author

Kraymer commented Dec 14, 2014

I prefer the function approach that ties the two regexes definitions as opposed to having to declare the two regexes. But I have no precise idea of what you intended by "global config option" so if you think it's the way to go, feel free to edit the code.
Or if you think of a better location concerning where to put that function?

@sampsyo
Copy link
Member

sampsyo commented Dec 15, 2014

Well, I actually tried putting together my regex-config-option idea and I didn't end up liking that much either. 😃 You're right that there's so much duplication that it feels futile.

Or if you think of a better location concerning where to put that function?

Sadly, that's the problem—we don't currently have a place for such things. I think you're right that this needs to go in util unless we invent something new: some kind of global beets context module. Bright ideas would be appreciated here…

Thanks for your patience. There are a few minor things I'd like to fiddle with—would you mind pushing this to a branch that I can commit to?

regex = r'(%s)' % '|'.join(['\\b%s\\b' % re.escape(x) for x in FEAT_WORDS])
if for_artist:
regex = r'(%s|%s)' % \
('|'.join([re.escape(x) for x in FEAT_SPECIAL_CHARS]), regex)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason that these "special chars" get treated differently? It seems to me like they should get the same \b treatment as the other words.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought too at first sight but

\b Matches the empty string, but only at the beginning or end of a word. A word is defined as a sequence of alphanumeric or underscore characters [...]

so these tokens are not words, and \b don't work with them.

@Kraymer
Copy link
Contributor Author

Kraymer commented Dec 15, 2014

@Kraymer Kraymer closed this Dec 15, 2014
@sampsyo
Copy link
Member

sampsyo commented Dec 16, 2014

Awesome; thank you for the extra effort!! 🐟

sampsyo added a commit that referenced this pull request Dec 16, 2014
@sampsyo
Copy link
Member

sampsyo commented Dec 16, 2014

Okay; I just made a few small changes and pushed. Thanks for pointing out the issue with \b; I've switched to using look-ahead and look-behind to sidestep the problem.

@Kraymer Kraymer deleted the 1060_ft_lang_support branch February 13, 2016 12: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.

2 participants