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

Fixup & rework many issues introduced by cloud #1100

Merged
merged 3 commits into from
Nov 20, 2022
Merged

Conversation

Pablete1234
Copy link
Member

@Pablete1234 Pablete1234 commented Nov 19, 2022

Not long ago #1080 was merged, and while it was a step-up some things have been missing, broken, or wrong ever since.

This addresses quite a long list of things in PGM:

  • Implemented an injection service for match modules, this means any arbitrary match module can be simply requested in a command method and it will be provided.
    • Some use cases are things like /join which request a JoinMatchModule or /class which requests ClassMatchModule.
  • Implemented generic parser for match objects, allowing easy re-using of logic for getting "match objects".
    • This is used by the parsers for Team, PlayerClass, or Mode.
  • Implemented generic Provider for Match objects, similar to the above but for injection instead of parsing.
  • Implemented option for MatchPlayer parameter parser to default to the sender, used in /variables [target].
  • Reworked /variables to paginate by default, allow showing all, and querying for names.
  • Reworked /mode start <mode> [time], now it parses modes by id instead of index in list, and can be autocompleted.
  • Made PGM modes always have an id. If XML does not provide one, pgm will autogenerate one.
  • Reworked & reintroduced legacy /rot commands, they filter by type and have additional help for the user.
  • Reworked & fixed setting /toggle commands, they support multiple aliases again, and autocomplete values based on key.
  • Reworked /pgm, to reload configs you now use /pgm reload, that frees-up pgm confirm and pgm help, which otherwise just get no-permission errors for default users.
  • Fixed vanished players not appearing in tab-complete list for OPs.
  • Fixed player being kicked when tab-completing usernames due to network error, caused by wrong nick resolution.

Additionally, it switches to a custom cloud fork i've made to fix other issues. The fork source can be found in https://github.com/Pablete1234/Cloud/tree/1.8.0-pgm, it's served from the maven repo https://repo.pgm.fyi/ and includes fixes for:

  • Predictable injection order (Make injection order predictable Incendo/cloud#402)
    • Without this, any command asking for an Audience could arbitrarily be served a Match or a MatchPlayer
    • While this is technically not a problem, as both Match and MatchPlayer implement audience, this has the side-effect of everyone receiving the command output (ie: if the command calls audience.sendMessage on an Audience being implemented by Match), or console not working at all, since it tries to resolve a MatchPlayer to feed as Audience, and ends with a not-a-player error.
  • Flags everywhere support (Add option to allow flags anywhere after last literal argument Incendo/cloud#395)
    • Without this, commands like /cycle 0 -f map do not work
    • By default, cloud requires flags always at the end, ie:/cycle 0 map -f
    • This also broke the vote book, since that relies in /vote -o map

@Pablete1234 Pablete1234 added the bug Something isn't working label Nov 19, 2022
@alexsosnovsky
Copy link
Contributor

With regards to the filtered rotation commands, the discussion that took place on the previous pull request must not be ignored prior to the decision of whether or not to approve this new PR.

It's linked above, I'll link it again. #1094

@Pablete1234
Copy link
Member Author

Pablete1234 commented Nov 20, 2022

In my eyes the discussion in #1094 already reached a conclusion: it's not worth continuing as the debate has already drained enough time & energy from us all, and it's clear you're not interested in reaching a consensus or proposing any alternatives that could satisfy both parties.

Within this pr, it has a solution among many other fixes, if it is accepted, we can finally move forwards and leave it behind. If it is rejected, pgm will have to find a new maintainer.

@alexsosnovsky
Copy link
Contributor

alexsosnovsky commented Nov 20, 2022

In my eyes the discussion in #1094 already reached a conclusion: it's not worth continuing as the debate has already drained enough time & energy from us all, and it's clear you're not interested in reaching a consensus or proposing any alternatives that could satisfy both parties.

Clearly the opinion is not unanimous. I believe that discussion has run its course and there are ample points made by both sides to be considered. Hence, that discussion shouldn't be forgotten due to the change in PR. That doesn't mean it needs to be debated further.

Within this pr, it has a solution among many other fixes, if it is accepted, we can finally move forwards and leave it behind. If it is rejected, pgm will have to find a new maintainer.

If a certain aspect of a PR is found to need changes, that doesn't mean the entire PR needs to be thrown out. Not sure why it's being posed as an all-or-nothing situation with the other cloud fixes, but that's not how reviews typically work to my understanding.

Copy link
Member

@Electroid Electroid left a comment

Choose a reason for hiding this comment

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

Thanks for working to clean this up, looks good!

@Electroid Electroid merged commit 14b64cc into dev Nov 20, 2022
@Electroid Electroid deleted the fixup-cloud-issues branch November 20, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants