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

Refactor/naming #65

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Refactor/naming #65

wants to merge 4 commits into from

Conversation

F-Shahali
Copy link
Collaborator

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Based on the conversation in #57 the refactoring is done.

Any other comments?

@F-Shahali F-Shahali added the fix label Jan 22, 2025
@F-Shahali F-Shahali added this to the mybutton v0.6 milestone Jan 22, 2025
@F-Shahali F-Shahali requested a review from AHReccese January 22, 2025 19:53
@F-Shahali F-Shahali self-assigned this Jan 22, 2025
Copy link
Member

@AHReccese AHReccese left a comment

Choose a reason for hiding this comment

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

Thank you for your effort.

Take a quick look on the TypeScript style guide.
image

Ref: https://ts.dev/style/#identifiers


export const services_url = (url: string, subject?: string): { [key: string]: string } => {
export const serviceUrls = (url: string, subject?: string): { [key: string]: string } => {
Copy link
Member

@AHReccese AHReccese Jan 22, 2025

Choose a reason for hiding this comment

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

use SERVICE_URL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this constant is indeed a function (processes input and the result varies depending on the input), wouldn't it be better to use the lowerCamelCase style?

Copy link
Member

@AHReccese AHReccese Jan 23, 2025

Choose a reason for hiding this comment

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

go with getServiceURL
@F-Shahali

};
};

export const Services = [
export const services = [
Copy link
Member

Choose a reason for hiding this comment

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

SERVICES

@AHReccese AHReccese added the enhancement New feature or request label Jan 22, 2025
@F-Shahali F-Shahali requested a review from AHReccese January 23, 2025 16:42
@AHReccese
Copy link
Member

I'm fixing the issue with the failed GitHub action (it is up now as a separate pull request) after that I will review this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants