mcstatus icon indicating copy to clipboard operation
mcstatus copied to clipboard

Add `clean` method to `motd` field

Open PerchunPak opened this issue 2 years ago • 9 comments

This will do easier to answer on questions like this or this.

I also will port this to #306 when will resolve merge conflict from master.

PerchunPak avatar Jun 19 '22 17:06 PerchunPak

>>> status = JavaServer.lookup("hypixel.net").status()
>>> status.description
'                §aHypixel Network §c[1.8-1.19]\n            §d§l3RD SKYBLOCK ANNIVERSARY'
>>> status.description.clean()
'                Hypixel Network [1.8-1.19]\n            3RD SKYBLOCK ANNIVERSARY'

PerchunPak avatar Jun 19 '22 17:06 PerchunPak

Looks good to go, though it may be nice to add some unit-tests before merging into master

I'm sure, regex isn't a something, that should be tested. I firstly thought about adding parametrize, but this unnecessary. I will add one test just for coverage.

PerchunPak avatar Jun 23 '22 08:06 PerchunPak

Perhaps we should take this further and make a class dedicated to parsing Minecraft text.

As I wrote in discord, this is just fast implement of one feature, that I come up with. I already made #335 for html, xt256, ansi etc. colors and more complex MOTD parsing.

PerchunPak avatar Jul 05 '22 16:07 PerchunPak

So it's approved, maybe merge it?

PerchunPak avatar Jul 17 '22 19:07 PerchunPak

So it's approved, maybe merge it?

There was a requested change from @kevinkjt2000 which even though you commented on as addressed, kevin didn't yet submit an approving review, dismissing the requested changes review status. I like sticking to a policy of PRs having no requested changes from maintainers before going through with a merge, so even though I approved, I won't merge the PR until kevin makes an approving review too.

ItsDrike avatar Jul 17 '22 19:07 ItsDrike

To be clear, I do see that #335 exists and I still believe that the new code should land in its already planned future home. Why would we add PingResponse.Description to overhaul that immediately later, when we could add a new class that is later extensible in #335 for the rest of the planned methods?

kevinkjt2000 avatar Aug 06 '22 15:08 kevinkjt2000

To be clear, I do see that #335 exists and I still believe that the new code should land in its already planned future home. Why would we add PingResponse.Description to overhaul that immediately later, when we could add a new class that is later extensible in #335 for the rest of the planned methods?

Maybe it should be in #335, but #335 is blocked by #306, which is waiting for your review for almost 1.5 months. This feature has also been requested several times on Discord, so should we delay a small (and requested) change for so long?

For now, let's at least avoid putting a class inside of another class.

When editing legacy code, follow the legacy structure. I'll change it in #335 (after merging #306) to the new structure, but for now let's not create chaotic code.

PerchunPak avatar Aug 11 '22 09:08 PerchunPak

When editing legacy code, follow the legacy structure.

No. By this same logic, I would never do chores. While living in a dirty house, keep living in a dirty house... We have already moved some other nested classes away from being nested classes. Indeed, we should not add more. There was a lengthy discussion about how nesting classes is unpythonic in the past. # THIS IS SO UNPYTHONIC is still in the code in fact.

which is waiting for your review for almost 1.5 months

I am slow to review, but at the moment I see the do-not-merge label and conflicts that need to be resolved on #306. Personally, I skip over PRs that still have changes outstanding from the checks that I see as an "automatic grader". This way I can optimize my time to review PRs that are ready. Also, this PR has conflicts too.

so should we delay a small (and requested) change for so long?

Yes, if that feature is not polished enough yet. Users expect quality even if they never ask for it. Open source does not have to be rushed, there is no money on the line here.


I am not saying to add all the planned functionality to that class yet. I am suggesting placing the class into its final destination from the start and avoiding rework.

kevinkjt2000 avatar Aug 16 '22 16:08 kevinkjt2000

Indeed, we should not add more.

I would like to fix it in specified for this place - #306. It would be worse to put it near PingResponse class (instead of in it) and create some chaos in code.

PerchunPak avatar Aug 17 '22 14:08 PerchunPak

Let's make the placement correct from the beginning... Why are you insisting on making a mess (that you already plan to cleanup) with the initial placement of this brand new class? I'm saying do that bit of cleanup now. Why wait?

Messy vs. Clean, clean wins.

kevinkjt2000 avatar Aug 23 '22 14:08 kevinkjt2000

Why wait?

Because if not follow current style, code will be messy more, than if we would follow the style. Even if it's ugly.

PerchunPak avatar Aug 24 '22 17:08 PerchunPak

However, I will move this PR to #335. (oh wait, I did it already in Motd.plain...)

PerchunPak avatar Aug 24 '22 17:08 PerchunPak

oh wait, I did it already in Motd.plain...

Almost as if placing this code into the new class in the correct location was better in the first place... I think you are catching on :)

kevinkjt2000 avatar Aug 29 '22 16:08 kevinkjt2000