Skip to content

Conversation

@moksha-hub
Copy link

Problem

Ticket No: #[BB-797]

@moksha-hub
Copy link
Author

@MonkeyDo Thanks for your help and guidance, and now my first PR is successfully awaiting approval to be merged. Once again, thank you!

@moksha-hub
Copy link
Author

@MonkeyDo i request you to have a look at my pr for any further corrections if needed.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay!

Thanks for rewriting this file, works like a charm! 🚀

I took the liberty of adding back the documentation for the allowOnlyGetMethod function, which I assumed were deleted by mistake.

Expecting the wrong key name in tests
@MonkeyDo
Copy link
Member

I had to dig a little bit to figure out why the tests were failing. You fixed an error with the expected key name ("relationship" vs. "relationships") here and the test was expecting the incorrect key and needed to be fixed.

@moksha-hub moksha-hub requested a review from MonkeyDo February 27, 2025 15:16
@MonkeyDo
Copy link
Member

MonkeyDo commented Feb 27, 2025

@moksha-hub for ease of work, you can run the linting on your local machine and avoid the back-and-forth, with the command yarn run lint
See installation guide if needed: https://bookbrainz-dev-docs.readthedocs.io/en/latest/docs/installation.html

@moksha-hub
Copy link
Author

I sincerely apologize for the multiple attempts. I appreciate your patience and understanding. Can you please check it now? Thank you for your time!

@MonkeyDo
Copy link
Member

No worries about the commits, I will squash it all into one commit when merging. It was more to simplify your own workflow :)
Will check again now.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Just a couple of minor details, some just linting warnings (not even errors).
After that, clear for merging ! :)

}, []);
}
catch (error) {
const errorMessage = 'Failed to fetch browsed relationships';
Copy link
Member

Choose a reason for hiding this comment

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

It could be helpful to have the full error message here for debugging purposes:

Suggested change
const errorMessage = 'Failed to fetch browsed relationships';
const errorMessage = `Failed to fetch browsed relationships: ${error.toString()}`;

Comment on lines -21 to -22


Copy link
Member

Choose a reason for hiding this comment

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

These two lines shouldn't be removed, it created an ESLint warning
"Expected 2 empty lines after import statement not followed by another import (import/newline-after-import)"

export const identifiersRelations = ['identifierSet.identifiers.type'];
export const relationshipsRelations = ['relationshipSet.relationships.type'];

export const relationshipsRelations = ['relationshipSet.relationships.type']; // Fixed from 'relationship' to 'relationships'
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the comment is required here, unless I missed something.
As it is creating an ESLint warning, let's remove it.

Suggested change
export const relationshipsRelations = ['relationshipSet.relationships.type']; // Fixed from 'relationship' to 'relationships'
export const relationshipsRelations = ['relationshipSet.relationships.type'];

Expecting an array under relationships with an s, not relationship singular
@MonkeyDo
Copy link
Member

See I messed up with fixing the test suite as well :)

rename relationship to relationships, and make it an array as expected.
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