Skip to content

[Console] Extracting ProgressBar's format's magic strings into const #39042

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 5, 2020
Merged

[Console] Extracting ProgressBar's format's magic strings into const #39042

merged 1 commit into from
Dec 5, 2020

Conversation

CesarScur
Copy link
Contributor

@CesarScur CesarScur commented Nov 9, 2020 •
edited
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

The existence of these magic string is propagating outside the project on the user land while constructing a command:

$bar->setFormat('very_verbose');

Objective is to provide reusability and resilience to miss naming:

$bar->setFormat(ProgressBar::VERBOSE);

@CesarScur CesarScur requested a review from chalasr as a code owner November 9, 2020 21:46
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@derrabus derrabus added this to the 5.x milestone Nov 9, 2020
derrabus
derrabus requested changes Nov 9, 2020
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

The _nomax format strings appear to be purely internal. They don‘t seem to be valid input for setFormat(). Thus I don‘t think that we should define them as public constants.

Other than that, I think this change is a good idea.

@CesarScur CesarScur requested a review from derrabus November 10, 2020 01:32
@CesarScur CesarScur requested a review from derrabus November 10, 2020 20:15
@carsonbot carsonbot changed the title Extracting ProgressBar's format's magic strings into const [Console] Extracting ProgressBar's format's magic strings into const Nov 15, 2020
@carsonbot carsonbot changed the title Extracting ProgressBar's format's magic strings into const [Console] Extracting ProgressBar's format's magic strings into const Nov 15, 2020
@derrabus derrabus force-pushed the PROGRESS_BAR_MAGIC_STRINGS_TO_CONST branch from 3051c6c to a4b2606 Compare December 5, 2020 16:15
@derrabus
Copy link
Member

derrabus commented Dec 5, 2020

I have inlined the suffix constant again as @nicolas-grekas suggested.

@fabpot
Copy link
Member

fabpot commented Dec 5, 2020

Thank you @CesarScur.

@fabpot fabpot mentioned this pull request Dec 5, 2020
Closed
@fabpot fabpot merged commit df65ffe into symfony:5.x Dec 5, 2020
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@fabpot fabpot fabpot approved these changes

@OskarStark OskarStark OskarStark approved these changes

@derrabus derrabus derrabus approved these changes

@chalasr chalasr Awaiting requested review from chalasr

Assignees
No one assigned
Labels
Console Feature Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@CesarScur @carsonbot @derrabus @fabpot @nicolas-grekas @OskarStark