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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove explicit public access modifiers #354

Merged
merged 3 commits into from
Jan 22, 2024
Merged

Conversation

beeman
Copy link
Collaborator

@beeman beeman commented Sep 3, 2023

No description provided.

@beeman beeman changed the title refactor: improve code style refactor: remove explicit public access modifiers Sep 4, 2023
@beeman beeman marked this pull request as ready for review September 4, 2023 00:31
@beeman beeman requested a review from Harpush September 4, 2023 00:31
@Jefiozie
Copy link
Member

Hi @beeman, just checking in with you, could you provide me a status on this PR?

@Harpush
Copy link
Collaborator

Harpush commented Jan 21, 2024

@Jefiozie it got stuck due to a discussion we had what public meant.
There are two options:

  1. Public has no meaning and should be removed.
  2. Public should be used for explicit public api while non public is used for angular implicit public methods like ngOnInit.

@SanderElias
Copy link
Contributor

Well, this is about code esthetics, as there is no actual difference in having the public in place.
I would simply remove all those keywords as its just visual noise.

Copy link
Contributor

@SanderElias SanderElias left a comment

Choose a reason for hiding this comment

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

LGTM

@Jefiozie
Copy link
Member

Before merging this we should resolve the conflict.

Also I would opt to add a eslint rule that explicit defines what should be defined and what not. In this case I would also opt to remove all public keywords.

I will add a comment on the linting issue to make sure that we add this rule.

Copy link
Member

@Jefiozie Jefiozie left a comment

Choose a reason for hiding this comment

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

LGTM

@SanderElias
Copy link
Contributor

@Jefiozie Done, also removed a couple more stray 'puclic''s ;)

@Jefiozie Jefiozie merged commit b616fbd into main Jan 22, 2024
1 check passed
@Jefiozie Jefiozie deleted the beeman/code-style branch January 22, 2024 17:50
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

4 participants