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

pr fixing issue [abbreviate_artistsort] exception when artist sorted name has no last name part issue#350 #365

Closed
wants to merge 0 commits into from

Conversation

sambhavnoobcoder
Copy link

added check to see if the artist name has a surname or not , and in case the exception occurs , the code recognises the name as a single word sorted name , and then the code generates initials for this single word to serve as an abbreviated representation.The initials are formed by taking the first letter of each part of the single word and appending a dot ('.') after each letter.The code then constructs a new sorted name by combining the original single word (treated as the forename) with its generated initials, separated by the predefined separator.Additionally, the code maintains the logic for handling whitespace and building the new unsorted name in alignment with the adjustments made to the sorted name, ensuring consistency between the sorted and unsorted names.

@Sophist-UK
Copy link
Contributor

Hi @sambhavnoobcoder

In #350 you asked me to review this, however I wrote it so long ago and the code DIFF has such big changes that I am not able to review it without spending a reasonable amount of time on it, and I simply don't have that time available. Sorry.

@sambhavnoobcoder
Copy link
Author

okay , so I'll consider it done from my end for the timebeing and move to some other issue for now , and whenever you find the time , just review the pr and if you find it satifsactory, merge it with the main project .

@Sophist-UK
Copy link
Contributor

I am not going to find the time in the foreseeable future - and in any case I am not an admin on Picard Plugins and cannot approve this.

@sambhavnoobcoder
Copy link
Author

okay . thanks anyway .

@Sophist-UK
Copy link
Contributor

There was no need to close this. Someone else will review it if you reopen it.

@sambhavnoobcoder
Copy link
Author

okay , i'll reopen

@phw
Copy link
Member

phw commented Jan 2, 2024

@sambhavnoobcoder For some reason I can't reopen this and see no changes. But please do submit a new PR. I'll do my best to review this.

I'm not familiar with the code and don't fully understand the functionality, though. I would have hoped Sophist-UK could review this. Do we have some test case in the MB database for which we can test the functionality and the bug?

@Sophist-UK
Copy link
Contributor

@sambhavnoobcoder For some reason you force pushed the original back to this PR so there are no changes and nothing to review.

@sambhavnoobcoder
Copy link
Author

ok i'll submit a new pr .

@Sophist-UK
Copy link
Contributor

  1. For the future, force pushes are generally not something you should be using for PRs.

  2. If you are going to submit a new PR, please try to localise changes to make it easier to review - perhaps make changes in several separate commits so that the changes in each commit are more localised and can be individually reviewed.

@sambhavnoobcoder
Copy link
Author

ok @Sophist-UK , ill keep that in mind from now on . I have made a new pr at #367 , it would be great if you and @phw had a brief look .

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.

None yet

3 participants