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

Added version number to ASCII banner #809

Merged
merged 6 commits into from
Nov 25, 2019
Merged

Conversation

Bilge
Copy link
Contributor

@Bilge Bilge commented Oct 18, 2019

Closes #808.

Copy link

@tests-checker tests-checker bot 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 contributing to Infection. We noticed you didn't add any tests. Could you please add them to make sure everything works as expected?

@maks-rafalko
Copy link
Member

maks-rafalko commented Oct 18, 2019

Can I suggest moving it one line below?

It looks prettier IMO, see

     ____      ____          __  _
    /  _/___  / __/__  _____/ /_(_)___  ____
    / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
  _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
 /___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/  0.14.0
 
     ____      ____          __  _
    /  _/___  / __/__  _____/ /_(_)___  ____
    / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
  _/ // / / / __/  __/ /__/ /_/ / /_/ / / / / 0.14.0
 /___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/  

definitely need another pair of eyes @infection/core

@theofidry
Copy link
Member

I would prefer to stick to the Composer way there tbh:

   ______
  / ____/___  ____ ___  ____  ____  ________  _____
 / /   / __ \/ __ `__ \/ __ \/ __ \/ ___/ _ \/ ___/
/ /___/ /_/ / / / / / / /_/ / /_/ (__  )  __/ /
\____/\____/_/ /_/ /_/ .___/\____/____/\___/_/
                    /_/
Composer version 1.8.6 2019-06-11 15:03:05

Did the same in Box btw in case you are looking for an easier way to copy it: https://github.com/humbug/box/blob/master/src/Console/Application.php

@BackEndTea
Copy link
Member

BackEndTea commented Oct 18, 2019

I think i'd prefer it below, like @theofidry suggests

@Bilge
Copy link
Contributor Author

Bilge commented Oct 18, 2019

OK, but do you just want the bare version number or do you want some decoration like, "Infection version x.y.z"?

@Bilge
Copy link
Contributor Author

Bilge commented Oct 18, 2019

Now looks something like

         ____      ____          __  _
        /  _/___  / __/__  _____/ /_(_)___  ____
        / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
      _/ // / / / __/  __/ /__/ /_/ / /_/ / / / / 
     /___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

Infection - PHP Mutation Testing Framework dev-master@2c915a4aa2c2b96a9ca2282254aebe8f7af29489

src/Console/Application.php Show resolved Hide resolved
src/Console/Application.php Show resolved Hide resolved
src/Console/Application.php Outdated Show resolved Hide resolved
Remove blank line after version output.
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Now it's much better :)

Found a small issue when I run bin/infection -V

bin/infection -V

    ____      ____          __  _
   /  _/___  / __/__  _____/ /_(_)___  ____
   / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
 _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
/___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

Infection - PHP Mutation Testing Framework dev-master@a1eb3df1012a2c86c0c32de9503e03140b1b7e9d
Infection - PHP Mutation Testing Framework dev-master@a1eb3df1012a2c86c0c32de9503e03140b1b7e9d

duplicated message


return parent::doRun($input, $output);
}

public function getLongVersion()
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove it?

I guess now it returns Infection - PHP Mutation Testing Framework dev-master@a1eb3df1012a2c86c0c32de9503e03140b1b7e9d instead of Infection - PHP Mutation Testing Framework dev-master?

Copy link
Contributor Author

@Bilge Bilge Nov 15, 2019

Choose a reason for hiding this comment

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

It was removed because it wasn't used. Or didn't seem to do anything useful. I don't exactly remember now 😂

@Bilge
Copy link
Contributor Author

Bilge commented Nov 15, 2019

What should happen when calling -V? Should it still output the ASCII banner or just the version number?

Changed version command output to exclude ASCII banner.
@Bilge
Copy link
Contributor Author

Bilge commented Nov 15, 2019

I changed the version command (--version/-V) to no longer show the ASCII banner. It is still shown for all other commands. My reasoning is that whether people wish to visually parse the version with their eyes or machine parse the version programmatically, they don't want to deal with the ASCII spam in either case. However, it still emits debug warning lines, which may also interfere with naive parsing.

image

I would suggest not emitting the debug warning with the version (-V) command or outputting it on STDERR rather than STDOUT to make it easier to filter, but probably either solution is out of scope for this particular PR.

@maks-rafalko
Copy link
Member

Sorry for the delay with review.

I'm quite happy with the result. Just 1 question: do we need that long hash in a version? Please compare

# before
Infection - PHP Mutation Testing Framework dev-version

# after
Infection - PHP Mutation Testing Framework dev-version@36f7fe04eca492c12ed5af23a4049c26214447f4

--

I agree, there is no point to display ascii logo with -V | --version

Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Please, fix the builds (make cs)

@maks-rafalko maks-rafalko added this to the 0.15.0 milestone Nov 20, 2019
@maks-rafalko maks-rafalko added DX Developer Experience Enhancement labels Nov 20, 2019
@Bilge
Copy link
Contributor Author

Bilge commented Nov 20, 2019

I presume the long hash is only displayed when the version isn't tagged. Most people would use a release version and thus never see it, and for those not using a tag it is probably useful to see the hash.

@theofidry
Copy link
Member

theofidry commented Nov 20, 2019

I would display the hash in any case, since sometimes a tag is not enough, but only showing the 8 first characters is enough, no need for more.

Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

I've restarted a couple of builds, fails are unrelated.

Looks good to me.

@Bilge
Copy link
Contributor Author

Bilge commented Nov 21, 2019

I think this should be merged to 0.13 and 0.14 branches also, so people using older versions can actually see that they are.

@maks-rafalko
Copy link
Member

We don't support 0.13 anymore. If you want this to be in the patch 0.14.x release, please retarget this PR to 0.14 branch.

@Bilge
Copy link
Contributor Author

Bilge commented Nov 21, 2019 via email

@theofidry
Copy link
Member

theofidry commented Nov 21, 2019 via email

@Bilge
Copy link
Contributor Author

Bilge commented Nov 21, 2019 via email

@sanmai
Copy link
Member

sanmai commented Nov 21, 2019

It makes all kinds of sense to run Infection only under the most recent version of PHP, primarily because of the most recent language features being fully supported, and because it is super easy with Docker.

As for the older versions of PHP, not only they're not very worthy for a CI tool like Infection, but also supporting unlimited number of PHP versions requires unlimited resources, which is unaffordable for any volunteer-maintained project like Infection. Or PHPUnit, for that matter. We already have important PRs in the works for weeks and months, as that's where ours and yours invaluable time and effort will make the most impact.

@Bilge
Copy link
Contributor Author

Bilge commented Nov 24, 2019

Nobody is asking anyone to support "unlimited versions". Notwithstanding there are not unlimited versions to begin with, and thus the whole argument is predicated on nonsense, can we just merge this and then I'll backport it to 0.14 in a separate PR.

@theofidry
Copy link
Member

theofidry commented Nov 24, 2019

But what you are requesting is to allow backports to previous versions which means multiple branch supports and management. That it may not look like much work to you is one thing, but really nobody in the team is willing to put extra efforts into this. We are already overwhelmed by the sheer amount of work to do (~50 issues and ~15 PR opened on average at all time).

A good example of this is PHP-CS-Fixer: its support policy is impressive, but the result is a huge slowdown in activity despite a lot of enthusiastic contributions. While it may be annoying for this peculiar case, we ought to draw a line somewhere for our own mental health

@maks-rafalko
Copy link
Member

Thank you @Bilge.

@maks-rafalko maks-rafalko merged commit 4ba7164 into infection:master Nov 25, 2019
@sanmai
Copy link
Member

sanmai commented Nov 25, 2019

Regarding a deleted comment on versions and testing: Infection is not a testing tool. If you need to verify that your program is correct as it can be, running tests under an earlier version of PHP should be enough. On the other hand, using Infection to infer quality of your tests under all deployed versions of PHP is a waste of resources since the only the most recent versions of Infection will give you the most accurate result. Earlier versions do not support all the new mutations, and therefore by definition are less accurate.

Running a most recent version of Infection just once in the CI pipeline is enough to tell you everything you need to know about certain quality of your tests.

@Bilge
Copy link
Contributor Author

Bilge commented Nov 25, 2019

@sanmai Thanks, that makes sense. Although it requires more effort to limit to only the latest build, it's probably worth doing.

Not sure any comments were deleted?

Bilge added a commit to Bilge/infection that referenced this pull request Nov 25, 2019
* Added version number to ASCII banner.

* Changed version number to long version. Moved version below logo.

* Changed logo to be left-aligned (no margin).
Remove blank line after version output.

* Fixed version output twice instead of just once when invoked with `-V`.
Changed version command output to exclude ASCII banner.

* CS fix.
maks-rafalko pushed a commit that referenced this pull request Nov 26, 2019
* Added version number to ASCII banner.

* Changed version number to long version. Moved version below logo.

* Changed logo to be left-aligned (no margin).
Remove blank line after version output.

* Fixed version output twice instead of just once when invoked with `-V`.
Changed version command output to exclude ASCII banner.

* CS fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Developer Experience Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infection should emit its version when run
5 participants