Restructure Command Mappings

Overview
This issue is for the discussion of refactoring the command mapping system within Sponge. The primary goal of this is to establish a clear, logical, and dynamic system to handle command registration and processing.

What's in a command?
Unfortunately, we don't have a standard definition of commands where we can easily identify a certain component we're interested in for every context. Before going any further, I'd like to address this problem.
This diagram shows the current command structure and how we refer to terms verbally, in text, or in code.

As you might imagine, this can get confusing - especially with commands that use multiple subcommands, multiple arguments (wait, does the one above have multiple arguments?), and so on. Therefore, I'd like to share the following structure instead.

Here are the terms I will be using and what I intend to mean by them:

Raw Command - the entire string (ex: time set day)
Command - the terms referring to a specific executor (ex: time set)

The command set may be treated as a separate command in the context of being a subcommand of time. One way to think about this is to consider time set the qualifier for set.

Base Command - the first command (ex: time)
Sub (Child) Commands - the remaining commands after the base command (ex: set)

A subcommand is only in reference to another command. In this case, set is a subcommand of time. This is because a command can be registered multiple times; it is that instance we intend to identify.

Argument - all terms that aren't a command (ex: day)

The plural (arguments) should only be used when referring to multiple parsed arguments. For example, the arguments of /tell Simon_Flash Hi friend! can be described as Simon_Flash and Hi friend!, though the argument is technically Simon_Flash Hi friend!.

Standardizing aliases
I'd also like to briefly touch on the subject of aliases. Outside of base commands, there are no checks to verify the assumptions we make when registering aliases. This means that a child command might be registered with SOME IMPPOSIBLE ALIAS.

The CommandManager contains checks that an alias is lowercase and does not contain a space. This check does not apply to child commands as they are not registered in the CommandManager.

I propose that aliases should follow similar requirements to a plugin id. They should be lowercase and have only letters, numbers, underscores, and hyphens. I don't see any reason why a command would start with anything other than a letter, but I don't see any reason to deny it either.
Additionally, an alias should always be considered relative to a parent - the time mapping is implicitly under the root mapping for base commands. The alias set also follows this rule, but in some contexts it could also refer to the subcommand of time. When one is not present, the root mapping should be presumed. This means time set can be used as a valid, uniquely identifiable mapping. For naming purposes, I will refer to this as a qualified alias.

Because the qualified alias is converted to a path of Strings using String#split, an empty / space only String results in an empty array that naturally references the root mapping.

What's the problem with the current system?
One way to consider looking at commands is viewing them as a tree. All commands originate from a root command (which can be represented by /) and move downwards through the base command and any subcommands. When it can no longer find any children, that executor is selected.
The time command in Minecraft has three sub-commands - add, query, andset, and the help command has none. Take a look at this diagram showing the theoretical mapping of these commands.

Here, it's clear that each and every command is it's own command. It has it's own CommandSpec (soon to be Command), it's own executor, and it's own child commands. This is how Sponge represents commands, right? Well, no, not exactly - the system we're working with looks a bit more like this on the outside:

This wraps any child commands into an executor that handles its own dispatcher, which for the most part accomplishes the same effect. However, in the process it obscures child commands, prevents them from being accessed once created (through the API), and blocks some of the major benefits a registered base command has that child commands should. This has also led to problems such as poor error reporting for child commands and has been an open issue for a while now (this specific problem is being addressed in the command refactor).
For my particular use case, I have a situation where I need to dynamically register new subcommands on top of existing ones. I've found a somewhat-workable solution via mixins, but as I'm sure we can all agree a solution within the API would be significantly better.

It is possible to access the SimpleDispacher inside through the use of reflection. However, actions like adding subcommands to a command without them would require restructuring the command to use a ChildCommandElementExecutor, which would effectively be rebuilding the backing CommandSpec all the way down to the base command. Messy, inefficient, and unlikely to work seamlessly.

Expanding upon mappings
One topic that's important to cover is whether a mapping should be defined by its parent or if a mapping should define its children. As in any node hierarchy, a mapping should be defined by its parent. This means that instead of having a Command[Spec] define children, it should define the parent(s) it has instead.
Mappings should also make use of the plugin namespace option that we use for base commands, that way if another plugin wants to register a command to time set players can use time minecraft:set or time myplugin:set - the set alias would basically be first-come-first-serve, but methods to replace the default should be made as well.
Finally, I'd also like to implement some way for users to tell us where the command stops and the argument begins. For example, something like /time: set day is interpreted as the command time with the argument set day. This allows players to be clear with the command they're trying to enter so we can direct it to the correct executor.

What's the solution?
API 7
There's no point in rewriting the mapping system for API 7 with the command refactor coming shortly after (not like it would be done in time anyway). While there may be some potential non-breaking changes that can be made, fixing the underlying problems would require breaking changes. If there is a desire to try and implement some of these changes for API 7, I'm more than willing to set up a PR.
API 8
The first step is redefining command mappings to represent an individual command rather than having the base command registered and an internal dispatcher for child commands. A mapping should be aware of the following information:

The aliases used
The Command[Spec] containing the executor, if it has one
Its child mappings
Its parent mappings*
The PluginContainer that owns the executor or, if there is not one, caused this mapping to be created.

The use of multiple parents allows the same instance to be used in both locations. Though this has some additional complications left up to implementation, this prevents instances from being desynced that should represent the same command implementation (same executor, aliases, and children)

For example, our time set day command would have a mapping described by the alias set, have an executor to parse day, no child mappings, and time as a parent. Notably, the mapping for time's parent is the root mapping provided by Sponge, which is the only mapping to not have a parent.
In addition to this redesign of the mapping system, I would also include the changes made to alias requirements, plugin namespaces for child commands, and identifiers to separate a command from the argument (see above for info).
Because the mapping system is recursive in nature and should support the same mapping being registered multiple times, there will need to be additional considerations to avoid things like loops or other weird effects. I will be researching more into node theory to understand potential pitfalls and develop solutions to them over the coming weeks.

Feedback
As this is a fundamental change for how we handle mappings, I'd like to ask for feedback regarding these topics discussed to get an understanding of what the community is looking for with such a system and whether these changes would be beneficial. If you have experience working with the internals of commands, I would greatly appreciate your thoughts on this matter.

Could you please provide some sample code for your suggestion?
Otherwise the suggestion sounds to vague.

So basically, what you really want it to be able to add your own sub commands to other commands that you do not necessarily own. Your use case is that you want to effectively hijack a command and warp how it might work through the use of subcommands instead of leaving the other command intact.
Take the /tell command. Traditionally, you would run /tell player this is a message to send a message to a player. Now, Nucleus overrides /tell, but then let’s say plugin X adds the subcommand player. Now, /tell player this is a message no longer works as expected, we have effectively legitimised the use of hijacking commands as such - you have subverted the intention of the plugin that owns /tell. By replacing /tell, you are taking ownership of that and making it clear that you own the command and thus dictate the logic. Players cannot be expected to write /tell: player this is a message. Sure, we have the send command event, but it’s not intended to hijack commands like this, more to prevent commands from being run for protection purposes.
If we did this, what about Brigadier commands? What about Forge commands? Commands that are not registered through Sponge’s high level API? For this to work and seamlessly as you’d like, we’d have to force everything to use our high level API - impactical. Yep, could do pattern matching when a command is run, but that could get expensive to run. What about if a subcommand doesn’t work? We currently drop back one level and try that (the source of the child command errors not showing, state is discarded). I don’t see this really improving the situation.
I personally cannot get behind this. I am firmly of the opinion that a command should own its entire tree from the root and all its children and arguments. You might argue it’s traditionalist, but I think that such a change will cause angst in the community if it was included, because you’d potentially get plugins adding their own subcommands to other plugin commands ending up with a myriad of problems. What if a plugin changes the command it uses? Should it affect just /time, or /minecraft:time too? What if Nucleus registers /time? Did you want it to hit Minecraft’s time only? Does it depend on Nucleus existing or not existing?
Root commands should be the only owner of its subcommands. This change would be intrusive and does not give many benefits.
As for the issue you linked on child elements and poor error reporting, I am in the process of giving plugins the option of what error can be reported back. The new command API will handle child commands, for the most part, differently because of this. That isn’t necessarily linked to this issue.
As an aside, SOME IMPOSSIBLE ALIAS is actually techincally possible as a child, you’d just wrap it in quotes.

I think this is less about hijacking and mutating the command tree, as opposed to providing some transparency to the tree built.
Sub-Commands are nearly impossible to provide valid help for currently, by a plugin external to Sponge.
I ran into this issue when attempting to expand BetterHelp into a system that could handle sub commands and custom documents.

@ST-DDT
Here is an example of you could add a new subcommand using the current API.
//The CommandSpec for the info subcommand
CommandSpec info = CommandSpec.builder().build();

//get the /sponge command mapping
Optional<? extends CommandMapping> optMapping = Sponge.getCommandManager().get("sponge");
if (!optMapping.isPresent()) {
    return;
}
CommandMapping mapping = optMapping.get();

//Next, we need to make sure that our callable is a CommandSpec so we can get it's executor.
 if (!(mapping.getCallable() instanceof CommandSpec)) {
    return;
}
CommandSpec spec = (CommandSpec) mapping.getCallable();

//Because child commands are contained inside the executor, we need to get the executor and verify it is a ChildCommandElementExecutor.
if (!(spec.getExecutor() instanceof ChildCommandElementExecutor)) {
    return;
}
ChildCommandElementExecutor executor = (ChildCommandElementExecutor) spec.getExecutor();

//We can register to this executor, but we can't get mappings. To navigate to the plugins mapping, we need to get the dispatcher.
CommandMapping pluginsMapping;
try {
    Field field = ChildCommandElementExecutor.class.getField("dispatcher");
    field.setAccessible(true);

    //Now, we can get the dispatcher which should be a SimpleDispatcher
    Object objDispacher = field.get(executor);
    if (!(objDispacher instanceof SimpleDispatcher)) {
        return;
    }
    SimpleDispatcher dispacher = (SimpleDispatcher) objDispacher;

     //Once again, get the mapping - this time for the plugins command
    Optional<? extends CommandMapping> optPlugins = dispacher.get("plugins");
    if (!optPlugins.isPresent()) {
        return;
    }
    pluginsMapping = optPlugins.get();
} catch (NoSuchFieldException | IllegalAccessException e) {
    //Well, back to the drawing board. Mixins are looking pretty good now.
    return;
}

//Now that we have the CommandMapping for plugins, we need to get our executor again.
if (!(mapping.getCallable() instanceof CommandSpec)) {
    return;
}
CommandSpec pluginsSpec = (CommandSpec) mapping.getCallable();

//It's a good thing that plugins has the reload subcommand, otherwise we'd have to rebuild the entire command to use a ChildCommandElementExecutor.
if (!(spec.getExecutor() instanceof ChildCommandElementExecutor)) {
    return;
}
ChildCommandElementExecutor pluginsExecutor = (ChildCommandElementExecutor) spec.getExecutor();

//With the executor, we should now be able to register our subcommand. Let's also check that it was registered successfully.
Optional<? extends CommandMapping> optInfo = pluginsExecutor.register(info, "info");
if (!optInfo.isPresent()) {
    return;
}

//Success... finally.

This proposal does away with the concept of storing mapping within the executor, which is the first and foremost thing to take away from this issue. Instead, mappings are stored in a more recursive manner. This can lead to solutions like the following:
//Our PluginContainer and CommandSpec for the command.
PluginContainer container;
CommandSpec info;

//Create a mapping for the info command. This holds the executor, aliases, and PluginContainer
CommandMapping mapping = CommandMapping.builder()
        .executor(info)
        .aliases("info")
        .build(container);

//Get the mapping for sponge plugins, which is where we'd like to attach our child to
Optional<CommandMapping> optPlugins = Sponge.getCommandManager().getMapping("sponge plugins");
if (!optMapping.isPresent()) {
    //At this point, we'd need to create the mapping for sponge plugins and potentially sponge if we want to move forward. This can be done, but for the purposes of this example we'll just return.
    return;
}
CommandMapping plugins = optMapping.get();

//Register our mapping as a child of the sponge plugins mapping
plugins.registerChild(info);

//If the sponge plugins mapping already had an "info" alias registered, it would not be overwritten. The "myplugin:info" alias must be used instead, though it should be allowable to replace the default alias.

Alternatively, we can provide some factory methods to handle the creation of 'empty' mappings (those without an executor). This means mapping instances are a bit more separated than I'd like, but it does mean we can use something like the following in place of the above method:
Sponge.getCommandManager().getOrCreateMapping("sponge plugins info", container).registerExecutor(info, container);

If you haven't already drawn a bit of a connection to Configurate, that's a great place to start. Mappings are like nodes, so something like getMapping("sponge plugins info") is similar to getNode("sponge", "plugins", "info") (split on " ").

@dualspiral
I understand and respect your concern for allowing other plugins to interfere with your commands. However, as @ryantheleach mentioned, the focus of this shouldn't be seen as 'hijacking' any registered commands but exposing the hierarchy they represent internally anyways.
Plugins can already unregister commands from the CommandManager and, if they so desired, do some serious damage to the server or other plugins. The environment we work in isn't necessarily one of strong security, so we work with the hope that developers will be responsible for avoiding potential collisions and being cautious with what they do. As my current solution to this problem is using Mixins to @Overwrite methods in SpongeCommandManager, it my desire to implement this functionality natively into Sponge to avoid compatibility issues down the road.
I think one of the large things to consider is whether child commands are their own command or should be considered part of the parent. We define child commands with their own executor; their own CommandSpec - yet still we treat them differently. We chose to implement a high-level command API to avoid the manual parsing of Strings as Minecraft and Forge do, so I don't understand why you would consider the line to be drawn here. I'm not familiar with the implementation of Brigadier, but I find it highly unlikely that it uses the raw String parsing as Minecraft did previously. If that is the case, then Forge should update to support that as well meaning there are no external barrier to requiring a higher level system. I agree that there is much more to discuss with this, but I don't see any reason why it isn't feasible.
I don't think I'm fully following what you're saying with regards to duplicate registration of the time command, but I'll give it my best shot. Minecraft has a time command by default, so that will be registered to the aliases time and minecraft:time. If Nucleus registers a command under the alias time as well, it will only register nucleus:time. If you would like Nucleus to override time to use nucleus:time instead of minecraft:time, you would do something like mapping.setDefaultAlias("time", nucleusPluginContainer).
I think the largest benefit of this system is that it's representative of how we think about commands. It's clear, easy to work with, and significantly more dynamic than the existing system. As I specified in the original post with regards to issue on poor error reports, I specifically mentioned (this specific problem is being addressed in the command refactor). You're correct in saying that this isn't the only reason why we had this issue, but it is a symptom of the underlying problem.
With regards to your aside, I don't think /tell: player this is a message is any more demanding than /tell "SOME IMPOSSIBLE ALIAS". Additionally, if I wanted to add in a subcommand with the current system I would need to register my own tell command, so now the player would have to use a namespace like /nucleus:tell which is a bit more verbose.
By defining commands in this manner, we can provide significantly more support and compatibility to anyone attempting to interact with child commands - and for some of the other features in the issue, commands in general.

My specific use case is for an alias plugin. I need to be able to add in sub commands dynamically that may end up replacing existing ones. For example, perhaps a server wanted to add a cooldown to time set. I previously listened into the SendCommandEvent and parsed things manually from there (I didn't allow for sub commands either, only the base command).

I think you are approaching this really differently then what you should be, I don't view needing to set a cooldown as needing to set or replace a sub-command, but intercept it when it comes to it's executor being called. If someone bypasses the Sponge API you will be doomed regardless, but lets just stay inside the bounds of SpongeAPI.
In addition, cooldowns can be flawed in that you don't know ahead of time if a command will succeed, so you can potentially end up in situations where the cooldown gets triggered, where it shouldn't be, leaving players frustrated.
I support having sub-commands be more transparent, but not to be dynamically manipulated from other plugins outside specific hooks that Sponge could be doing better to provide.
re: Conflicts
It's my belief that any conflicts should be handled through the global configuration system we have already, and that if it isn't enough, it should be expanded.
re: Brigadier
Brigadier views sub commands as passing in a literal, and treats them just like any other argument. see: https://gist.github.com/Dinnerbone/7370a2846953eee2d8fc64514fb76de8

The point isn't specifically the cooldown, so let's please not use that as a straw man argument (not that you're intending to, but it's not the focus of the discussion). There are, in my opinion, plenty of valid uses for such a system. Earlier this year when Nucleus' /back command required the --force flag due to a bug, I used this to override the /back command to become /nucleus:back --force, effectively solving that issue. I don't disagree that are possibilities for abuse or collision here, but there is also potential to support a lot of positive uses is a stable manner.
What specific issues do you see with having child commands being editable by plugins other than the one that registered it? Is this something that can be addressed by have a #lock method that prevents a mapping from having a default alias replaced?
By conflicts, I presume you mean cases where two plugins attempt to register the same alias. This is basically done by attempting to add both alias and plugin-id:alias - if alias is already registered, then (baring duplicates) plugin-id:alias is secured as the backup and is available, which is why minecraft:time works fine. This is a very simple and logical way to handle duplicate registration of commands and, considering it has already been well tested for base commands, should work fine for sub-commands as well.
Brigadier does seem to do manual parsing for subcommands, so I stand corrected on that. However, with access to the command_tree.json file (which, notably, matches the comparison I made of commands to trees) I believe I can generate the necessary executors and mappings to integrate Minecraft commands with this system. I can play around with it more when we have the source available to us, but I'll take it upon myself to be personally responsible for getting Brigadier to work with this mapping system should we proceed with it.
I'm going to decouple my mapping code and refactor it into what I'm trying to present here. Additionally, I'm also going to put together some examples using the system to show how potential pitfalls might be addressed and demonstrate some scenarios with registering commands.

For what it’s worth, when it comes to child commands with the refactor, recognising the problem, any command built using the builder will also be a dispatcher, decoupling children from the parameters. This will be part of SpongeManagedCommand. That way, it is possible, in Common, at least, to traverse the subcommand tree from parent to child. Not so much the other way because it is designed to allow you to use a child in multiple commands - but it’s better than now. That’s going to allow us to generate command trees too - a lot of the refactor was developed with command children in mind.
Commands should own their subcommands. I don’t see that as a role for the command manager.
If this is really wanted, I’d only go as far as exposing the Dispatcher interface on the API for a ManagedCommand. I, however, would also strongly resist this, however, because I stand by my statement that other plugins should not be messing with other commands. Yes, there has to be an element of trust (unregistering/registering/replacing commands, for example), but we shouldn’t add (or expose) the ability to do something that we don’t want plugins to ever do. This is one of those times - we should not be allowing plugins to dynamically register subcommands without the owning plugin’s consent.
The problem is not with the command manager, the problem was with child commands - and this is a problem I am already trying to solve in #1587. I appreciate why you’re suggesting this, but I don’t think that this approach is going to bring anything further to the table.
(My aside was a point, by the way, not suggesting something that you should be doing.)

If this problem could be solved by exposing the dispatcher in ChildCommandElementExecutor, this issue would be a PR instead. The dispatcher can easily be gotten through reflection; it's navigating through the executor that's the problem. If you prefer avoiding methods that encourage mutating others commands, then simply don't allow any editing to mappings once they're registered and have developers use reflection instead to gain access if they so desire. Treating commands as a tree like this is going to result is a more organized mapping system and make it easier to navigate through the system - in that manner, there are no negatives that have been brought up so far.
The focus has been too much on the fact that plugins that edit other's commands (which is part of my intention, yes) and is ignoring the actual redesign of the mapping system that is the center of this issue. This redesign opens up accessibility to subcommands - we can now tell exactly what subcommands a command might have, the permission you need to run them, and any other information we so desire. To relate to Configurate again, it's the difference between having to get every single node from the root node compared to the ability to save child nodes for reuse later. As far as the redesign is concerned, there are virtually no negatives and a large amount of benefits - I don't see how you could say that this approach isn't going to bring anything further to the table.
I've never implied that the problem was with the CommandManager; there's a reason I called out ChildCommandElementExecutor specifically. I'm aware that you've addressed this issue (as I mention in the OP as well as my response) through ChildExceptionBehaviors, but that's not the point of this issue. The goal here is to expose the mapping system it contains internally instead of walling it in where it becomes impossible to work with.
The idea behind having a separator where a user can define the start of the argument is literally only a positive change. The only way we could ensure that a child command cannot inflict with its parent's executor is to require that a command may either have children or and executor - not both. In those case (albeit rare) having that separator is everything.

I'm aware that you've addressed this issue (as I mention in the OP as well as my response) through ChildExceptionBehaviors, but that's not the point of this issue. The goal here is to expose the mapping system it contains internally instead of walling it in where it becomes impossible to work with.

I'd read what I said and read my PR again, because as I said, I've moved the dispatcher FROM the parameters TO the main Command - the implementation is SpongeManagedCommand. Every Sponge Command built using the builder will be a Dispatcher. We can easily build the tree using this approach, it is not going to be wrapped up in a parameter - that was one of my first goals, since I did the same in Nucleus so I could access the subcommands more cleanly. ChildExceptionBehaviors are just part of the story.
As I said:

For what it’s worth, when it comes to child commands with the refactor, recognising the problem, any command built using the builder will also be a dispatcher, decoupling children from the parameters. This will be part of SpongeManagedCommand. That way, it is possible, in Common, at least, to traverse the subcommand tree from parent to child.

Have a look at the proposed class here: https://github.com/SpongePowered/SpongeCommon/blob/d49591a4f42b60fdbcd423e549506f100ffa76f2/src/main/java/org/spongepowered/common/command/managed/SpongeManagedCommand.java

we can now tell exactly what subcommands a command might have

I can and will look at exposing that in my refactor, it's something that has been asked for and will be easy to do. I don't plan to allow arbitary subcommands to be added to a command, for the reasons I give above. Non-sponge built commands will not expose Dispatcher, however - the idea was that we'd just have a Command, but it's looking like to me that I will need to expose a CommandSpec analogue anyway.
Please, before telling me I'm not solving these problems, read what I write and run through my PR. It's over 10k lines in Common alone dedicated to solving a lot of the command problems. Such feedback would be valuable there, rather than talking about ripping apart the command manager for one use case. My refactor improves a lot of shortcomings including most of what you talk about here.

Seeing that class is definitely helping me to understand what you're getting at. I combed through as many relevant classes as I could find, however as you've said there's over 10,000 lines in just Common - chance of me finding everything and understanding it to the same degree is next to none, especially as I'm not the one who authored it.
I believe we're on the same page with regards to unboxing the storage of child commands and providing methods to work with accessing child commands and navigating through the hierarchy. My personal opinion is that an executor is a specific handler for a command. That handler, combined with it's permissions, aliases, and other settings form the framework of the command itself. Notably, that means a command is not defined by its children or its parent - it is, in and of itself, unconnected.
The connection between commands is what a CommandMapping should represent - each one refers to an individual mapping which has references to it's place in the tree through parent and child references. In cases of multiple parents, the same mapping instance should be used. This is akin to how you have children be inside the Command class themselves, which allows that command to be registered in multiple locations while retaining the same child commands - same idea and effect, just a bit more segmented.
My intention with this is to work with the creation of custom commands, but under no circumstances is this intended to manipulate the actual executor of the command. By decoupling the mapping from the command, it makes it easier to work with the structure of the tree without having unintended consequences. This is something that we can either provide some support for in Sponge, or require a lot of custom implementation that is less stable and not mediated in the same way. The distinction between a command and it's registered mappings is something that I want to emphasize because my intention is not to replace the actual command, but rather reroute aliases to other mappings.
As an example, let's work with Minecraft's time command and it's subcommands, particularly set, and we'll presume it's structured to work with subcommands over literal parsing for the purpose of this example. First, here's what the mappings would look like:
R -> T
T -> S

R = Root Mapping (children = {"time": T, "minecraft:time": T}
T = Mapping for time (parents = [R], children = {"set": S, "minecraft:set": S}
S = Mapping for set (parents = [T], children = {}, executor = the executor)

Let's say that I run a Spanish server and want to have command accept Spanish aliases as well. This can be done by adding a new alias to a mapping.
T.registerAlias(container, "tiempo");

R = Root Mapping (children = {"time": T, "minecraft:time": T, "tiempo": T, "myplugin:tiempo": T}
//remainder stays the same

Next, I want to have a simple alias - settime - that can be used instead of time set (spaces are bad and stuff). This is where it does get a little tricky, as if we register the entire mapping B to A we would end up with the command /set, which we don't want. Instead, we're going to register a specific child alias - just in English this time for simplicity.
R.registerChildAlias("timeset", S, container);

R -> S

R = Root Mapping (children = {"time": T, "minecraft:time": T, "tiempo": T, "myplugin:tiempo": T, "timeset": S, "myplugin:timeset": S}
S = Mapping for set (parents = [R, T], executor = minecraft executor)

Moving back to our desire to make our commands support Spanish, I now want to have set support the Spanish words dia and noche for day and night respectively. Because these are arguments, we need to register a new executor that will accept these variants. Because we're working with a new executor, that means we're going to be creating a new mapping to replace it.
CommandMapping C = CommandMapping.of(executor, container);
mapping.registerAliases("set", "conjunto");
T.registerChild(container, C);

T -> C

T = Mapping for time (parents = [R], children = {"set": B, "minecraft:set": B, "myplugin:set": C}
C = Mapping for Spanish set (parents = [T], executor = custom executor)

Notice that only myplugin:set was registered - set still refers to the mapping for the default Minecraft command. For our needs, we want to replace set with our mapping because it has our custom executor, so let's go ahead and do that.
T.setDefaultAlias("set", C);

T = Mapping for time (parents = [R], children = {"set": C, "minecraft:set": B, "myplugin:set": C}

First, note that minecraft:set is still available in case we would like to use it - if subcommands did not support namespaces, this would need to be manually set up. Doable, yes, but an unneeded expense. Second, it should not be allowed to set the default alias for a namespace - in other words, minecraft:set will always refer to the mapping container the executor owned by Minecraft.
If a plugin so desired we could add in a lock feature to prevent the default from being changed - though someone who has reached this point should understand what they're doing anyways. I would also suggest the possibility of having some type of method in Sponge to set the mapping for an alias, very similar to what we do in the config file for base commands, just on a subcommand level (in fact, this might be preferred over having a #setDefaultAlias method in the first place - food for thought).
With these examples in mind, this is why I would like CommandMapping to support this natively in Sponge. Having the security where a command can never be completely replace due to namespaces coupled with setting defaults in the Sponge config should help to alleviate some of the concern regarding the 'replacement' of commands. By implementing the mapping system separately, we can safely allow these operations without hijacking a command - just adding and replacing some aliases.

The thing is, this all goes back to manipulating other commands. I know you don't alter the executor of a command, but you are altering the expectation of a command.
If a plugin wants to create a spanish version of a command, it will likely create an entire command node in Spanish, with Spanish translations for all the command nodes. You wouldn't have /tiempo set day or /time set dia, for example, you'd want to translate the whole command line. That would be a different command.
What about those commands that have subcommands in string form? Take a forge command, it might have a /forgecommand sub1 command, but, in essence, is a handler for the /forgecommand. You could register sub1, and effectively blow away their subcommand because it becomes unreachable. You also have the potential to confuse users AND developers, if someone came to me to tell me /message player x isn't working because you took /message player but not /message, you've hidden the ability to run /message player x as the player part was not a subcommand.
This is the problem with this, and why I do not think this should go ahead. Commands that own their subcommands know exactly what they are doing. They know what possibilities they are hiding. If this was adopted, I could suggest that I want /time set day to be my command, override the day part to be a subcommand, and then turn day into night. Your answer here would probably be to use /nucleus:time set day, but that's not intuitive to the user, they are simply going to think it's a bug. This change would ultimately severely decrease usability, as well as breaking any expectation we can have with commands and subcommands.
Visibility of subcommands withing managed commands is not an unreasonable thing to ask for, interception of such commands using events to apply cooldowns etc. (as an example) is a reasonable and valid use of exposure of subcommands. A mishmash of commands and subcommands by different owners is only going to benefit a handful and serve to cause confusion for many. It doesn't matter whether or not you think putting a lock on the dispatcher would prevent that.
This would encourage bad practice and etiquette amongst plugins. There should be no expectation that a plugin will interfere with another's commands - this includes adding a subcommand that could change the behaviour of a command. We should not expect users to known to use a namespaced subcommand, or some token to say "ignore this subcommand". Users will complain to where they think a command/subcommand has come from if something goes wrong - the owner of the root command. I get enough of this when a plugin registers the same command as Nucleus, wins the default alias, and other plugins aren't expecting that to happen. Overall, I do not expect other plugins to mess with my commands - they should not at all be breaking the expectation of the owning plugin of the command and causing confusion.
This is only going to enable confusion.

If this is going to be so confusing, then how is it we've learned to work with this system for base commands yet cannot expand it to subcommands as well? You speak of translating the command line, and that's correct - but it's not the aliases that define how a command is executed. Nucleus overwrites many of the Minecraft commands, but this does not spark the same concern?
If you would like to use your example of /forgecommand sub1, then recall how the system processes this. If a valid subcommand mapping is found for forgecommand sub1, then the mapping will be selected first. This is absolutely no different from how a subcommand you would register handles the same process. If there is some need to attempt to prevent the mapping from being edited in these cases, then a lock can be applied to reasonable success (as at the end of the day those plugins that need to replace a command will find a way). Even a warning message when a mapping is edited by a plugin that didn't register it can be done to provide some type of notification to the player. There are many ways that this can be implemented in a reasonable manner to minimize any potential issue that I believe you overestimate anyways.
I know for a fact that you've dealt with conflictions because Nucleus' /home and /ftb home, so how did you handle these processes? I believe /nucleus info generally tells you the idea because it prints out all base commands, aliases, and the plugin/mod they're registered to. It is quite simple to have a /checkexecutor home command that gets the mapping for the alias home and lets the user know it's owned by Nucleus. As I've brought up, it is also easily feasible for us to add in a separator that can be used to specify where the argument begins - I'll admit that /forgecommand: sub1 is a bit of extra work, but it is not so much additional effort for it to justify denying these changes that could be largely beneficial.
Now, let me ask you what makes you believe that these changes are going to negatively impact so many people? I think we can both agree that the use of adding subcommands or other edits to command mappings is limited, so how expectant should players be that they're going to run into this? In my case, I supply a plugin that has the capability to do so to, and it's up to the administrators to make use of it as they see fit. If they would like to work with mappings to add in their own functionality, then that is their choice. You may say there's no reason to require this, but in the case of a scripting plugin I have certainly found a use for it. We can disagree about how allowable that should be, but I believe my users understand it is their responsibility to address potential conflicts and handle them accordingly.
If you're so concerned about users having difficulty with locating the source of the error, just add a registered by myplugin. note on the end of the error message. If you believe that players on a server should not be required to know about namespaced subcommands and stuff, I agree. But it is the administrators of the server, whom I believe should know about namespaced commands, who have the responsibility of ensuring that the default aliases function the way they intend, and we can easily provide that functionality in the Sponge config as I mentioned above. There are many other things that we expect users to work and understand beyond this, and I fail to see how you perceive it to have such drastic effects. It is only as difficult as your inexperience - like permissions, like custom gui menus, like kits, and like working with servers in general there is a learning curve. As layed out in the examples above, there are clear, logical, and straightforward rules to how the system works that can be documented - the confusion is only due to unfamiliarity.

@SimonFlash make simple proof of concept and we will test it.
If this will work only for sponge's high level command api - this will be completely useless thing.
Look at bukkit, bukkit is very popular, because it's easy as a pie.
As plugin developer, i want to say, that we want just write plugins, not enterprise level apps.
Atm sponge has very hight entry level, its really hard to understand sponge concepts for many programmers.(hello from non-enterprise world)
We need to make sponge much simpler.

Okay, I'm going to sum up exactly what I think here, because I think what I'm saying is now being twisted. The only reason I can see you arguing this is because you want to inject your executors as children into other's commands.

Nucleus overwrites many of the Minecraft commands, but this does not spark the same concern?

Yep, and I take ownership of that, the entire command line. I replace the default behaviour of the line, from the /tell to the message. If you start injecting subcommands into my command, you take ownership of part of the command, which I ultimately have the problem with. As I said, now /tell serverwide name.
The fact of the matter is that with subcommands, the expectation is, and should be, that the subcommand is a chld of the command. This implies ownership. Ownership should be of the entire command string, not part of it.

I know for a fact that you've dealt with conflictions because Nucleus' /home and /ftb home, so how did you handle these processes?

I didn't do anything. I gave users the information. I do not go and try to inject my command into their subcommand, because in that instance it is reasonable to assume that, /ftb indicates a FTB pack command. Developers should not be interfering with other's workflows. Replacing a command is acceptable, replacing a subcommand is not. Asking a user to run /home nucleus:set because some developer decided they wanted to replace /home set with some other function is not acceptable. You talk about having some sort of config, but how complex is that going to get?
My point is that any subcommand must ALWAYS be associated with the parent command, and that includes owner and expectation. Take your point:

If a valid subcommand mapping is found for forgecommand sub1, then the mapping will be selected first. This is absolutely no different from how a subcommand you would register handles the same process.

Yes, I know that, I'm writing the API for this. Also note what I said earlier:

Commands that own their subcommands know exactly what they are doing. They know what possibilities they are hiding.

This is exactly it. If I write the command, then write the subcommand, I have written my plugin with that in mind. I own ALL the logic for the command and all the subcommands. I do not expect a plugin to come in and pull that rug out from under me and break my own command. A plugin at runtime that takes a subcommand only, I guarantee, will NOT check what they are replacing and will end up in a situation where the subcommand does not fit with the root command.
It would not be fair to say I don't understand what you want to do. I do - but it is a lot of work for little gain and the potential for a lot of abuse and confusion (and I would say it is from the developers side too - I think that the current, and possibly the proposed, high level command API is complex as it is - yes, I want to try to make what I'm doing simpler.) You talk about documentation, but @XakepSDK has a good point. This is serving to make Sponge an even more difficult system to work with.

Now, let me ask you what makes you believe that these changes are going to negatively impact so many people?

I've explained this before - please read all my comments again. I should add that there is also a maintainance cost asscoiated with this. The command manager cannot, and shoud not, manage subcommands, that's what the child dispatchers are for. I'm correcting that logic in the dispatchers, and I am willing to add a method like getCommand(String...) to get a subcommand out. And then the config, that would be somewhat complex to teach to a younger user who just wants to play Minecraft with his friends. And before you say that your use case isn't for them - what about the plugin that intecepts my command in the future and does something different, because it didn't think about the fact that other plugins might have the same command? HomeSweetHome might be expected there, a user uses Nucleus' homes, commands are different, things go wrong.
Sponge is already complicated as it is. You might see this as trivial, but for some people, no amount of documentation is going to help. This adds complexity where there really does not need to be - we already suffer in other parts of the API (hands up, command API is the same and I'm still trying to think about ways to simplify it), custom data, I'm sure there are other examples where developers aren't using them because it's just too complicated. This would be yet another example of it, but I think it's an example that could cause a lot more damage because it would just end up being poorly understood and impacting on other plugin workflows. At least with the standard command API and data API, they are self contained within themselves, a plugin that does something wrong is much less likely to break another plugin.

You may say there's no reason to require this, but in the case of a scripting plugin I have certainly found a use for it.

Yes, this is the crux of the matter. You want this so you can intercept and replace/reroute command logic and what I see as subvert the will of other plugins. In time, plugins would just use any lock we gave them to prevent other children being added, and we're back to square one, a complex system for what becomes no gain whatsoever. Plugins should not have to expect their commands to be taken apart like this.
My opinion is known, and I think that setting subcommands outside of the context of the owner of the parent should not be done - the command manager is fine as it is (the CM should not be responsible for child commands) and the ownership of the subcommands should be on the child dispatchers, owned by the parent plugin. We should not be tampering with other's command structures. Replace the structure, yes - there is no problem there because you are taking that ownership of the entire command string, not part of it. Don't tamper with other plugin's expectations like this.

@XakepSDK
I've already started separating the mapping system I use and plan on finishing it soon. It will be able to support plugins who still (for whatever reason) choose to support raw string parsing, but it would naturally be less effective than those using the high-level Command API.
While the changes presented here do sound a bit more complex, I don't personally find any more difficulty working with the system as I do with the current CommandSpec class, and I believe that with assistance and support it can be integrated into Sponge in a logical and effective manner. The intention here is to make Sponge simpler, and for the same reasons dual has updated the Command API of Sponge I believe the mapping system should be updated as well.

@dualspiral
Yesterday, I had this discussion with @Rasgnarok regarding the how these changes would affect existing commands and how the user can be made aware of cases where a mapping has been edited outside the original plugin. I think it's a good read and talks about many of the concerns with the system, and hopefully provides a reasonable solution in response.

This is exactly it. If I write the command, then write the subcommand, I have written my plugin with that in mind. I own ALL the logic for the command and all the subcommands. I do not expect a plugin to come in and pull that rug out from under me and break my own command. A plugin at runtime that takes a subcommand only, I guarantee, will NOT check what they are replacing and will end up in a situation where the subcommand does not fit with the root command.

This presumes that a plugin decides for some random reason to just add their command under another because they would rather have that alias, and that isn't how the system should be all. Anyone, developer or administrator, who decides to add a new subcommand or overwrite a default alias should have a reasonable expectation of how that might affect the parent command and what the potential downsides should be. People should not be combining commands together for the sake of having them under a singular alias, but should rather be building upon the existing command in a safe and logical manner. Thinking that someone will add a subcommand without regarding for the existing structure is, in my opinion, a mistake.

And then the config, that would be somewhat complex to teach to a younger user who just wants to play Minecraft with his friends. And before you say that your use case isn't for them -

My use case may not be for the user just looking for something simple, but it is most definitely for the server owners who wish to customize their server and give commands more functionability. All that's needed to do to change time set to mean time myplugin:set is something as simple as this in Sponge's global.conf file:
commands {
    # A mapping from unqualified command alias to plugin id of the plugin that should handle a certain command
    aliases {
        "time set"="myplugin"
    }
}

We can address many of the potential issues before they develop by providing appropriate warning messages to console when a command is changed and including info about the command in any error messages, particularly the plugin responsible for it so administrators know who to report to.

This would be yet another example of it, but I think it's an example that could cause a lot more damage because it would just end up being poorly understood and impacting on other plugin workflows. At least with the standard command API and data API, they are self contained within themselves, a plugin that does something wrong is much less likely to break another plugin.

That is true, but it is also something that could be addressed rather than downright prevent this addition. I maintain the mentality that a developer registering a subcommand should have an understanding that they are attempting to attach their command to an existing one. I can demonstrate this further once I have the example mapping system I mentioned above set up, but a plugin who is registering commands normally will never have to interact directly with this system in the slightest.

You want this so you can intercept and replace/reroute command logic and what I see as subvert the will of other plugins. In time, plugins would just use any lock we gave them to prevent other children being added, and we're back to square one, a complex system for what becomes no gain whatsoever. Plugins should not have to expect their commands to be taken apart like this.

I'd like to note again that in my case this is for server owners to work with, not the plugin directly. It is their decision how they would like to change the command system, but I am asking for this system to be implemented in Sponge so I can ensure the necessary contracts to minimize any undesired side-effects. Plugins do not have to expect their commands to be take apart; the responsibility for ensuring any edits work should lie with the developer or administrator making those changes. It is still my belief that this system would be primarily used by the server as opposed to developers, and I would like to have it in Sponge so I can do so as safely as possible.
I suggest that for now we table any discussion until I have the opportunity to include an example usage of this system so you can have an understanding of how it works and what actions are exposed to developers. I hope to have that available within the next few days.

I'd previously left this open because I felt that it was worth discussing with other Sponge members, I am not the only person on this project and so my opinion shouldn't be the only one that counts. I went and asked the opinion of some of the Sponge team about this, and our collective feeling is that a command should only be owned by the plugin that registers it, this includes subcommands. We do not want to provide the ability for a plugin to override another plugin's intention using subcommands, there is a general belief this is overstepping the mark.
I am working to expose subcommands through the command API refactor - that's worth merit, so that you can see what is registered, and that something like @ryantheleach's BetterHelp would become possible. Creating subcommands, however, will remain the sole right of the plugin that created the parent command.

Fair enough, although there are still components to this unrelated to editing subcommands. I'll play around with what you've done for API 8 when we start getting into experimental builds and see if I'll be able to implement what I need without overwriting the existing system as I do currently.

@SimonFlash if you need inspiration, or hell, would like to port an existing effort, looking at https://github.com/EngineHub/CommandHelper it's made to be somewhat platform independent, and there was interest in the very early days of a potential port to Sponge. It's also really nicely engineered.

@ryantheleach
Thanks for the reference, but to be perfectly honest I really don't know what I'm looking at. I do already have the majority of the implementation planned out for this, so I'm going to continue drafting that up and attach it here when it's complete in the hopes that this issue will be reopened, as I believe there was far too much focus on the idea of adding subcommands that the point of the issue was lost.


How to use Bitbucket as VCS

When using BitBucket as VCS, AFT deployment is not creating any Code Star project. Is there any specific steps to establish connection with BitBucket

Resolved, it was actually in connection settings.


raidboss: show invuln/provoke/shirk triggers to healers

Fixes #1884.

I already have these turned on for all jobs because I care about that sort of thing, but I am not sure if everybody else wants them too.  Anybody have additional opinions one way or the other?

For myself, I prefer knowing when tanks are swapping or using invulns. It's usually going to happen around nebulously Dangerous Mechanics, and being warned helps me as a damage dealer to look out and stay out of the way. (Obviously this is already covered a lot of times by tankCleave, but it can still be helpful.)


Need clarification on foreach loop warning

HI,
I was curious what exactly "Nested by-reference foreach loop, make sure there is no iteration over the same array" warning mean?

Hello.
There are very subtle backward incompatible changes when the array that is iterated over is modified inside the foreach loop. Here is an example that demonstrates the difference (taken from the foreach RFC).
The warning is poorly worded. It should probably say something along the lines of: "Nested by-reference foreach loop, make sure that array modifications (if any) are safe


[Feature Request]  Add support for RNDIS (networking over a wired USB cable)

I would like to see support for wired networking over a USB cable similar to what Rogue on the rg350 has.  I am not sure if all the kernel drivers are available for it but if they are it would be nice to have a menu option to enable/disable rndis over usb like there is for wifi.

Not something I will invest time in at this point.  It may be something I review in the future.


fix pre-release-tests.yml to use v2.15.1 and 2.19.0 like the rest

Pull Request
Related issue
fixes the reason https://github.com/meilisearch/meilisearch-dart/pull/267 is failing CI
What does this PR do?

Fixes pre-release-tests.yml to use 2.15.1 and 2.19.0 of the dart sdk

PR checklist
Please check if your PR fulfills the following requirements:

[ ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
[x] Have you read the contributing guidelines?
[x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

Thanks a lot for this contribution as well ✨

Thank you @ahmednfwela you rocks! :D


move lint actions to their own job, add more checks, fix linting issues

an external contributor snuck some new dependencies in without updating package-lock.json, that sort of violation will now be caught in CI.
another external contributor helped us follow the GitHub markdown accessibility guidelines, but didn't add a CI action to ensure we keep up to date. This adds markdown linting, and most violations have been fixed

I decided to disable the line length rule. It's 2023 and screens are somewhat wider than 80 characters. Maybe in the future we'll turn it on with a higher limit

Codecov Report

Merging #85 (375e86b) into main (eee2559) will increase coverage by 2.27%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
+ Coverage   80.62%   82.90%   +2.27%
==========================================
  Files          15       15
  Lines         986     1000      +14
  Branches      100      140      +40
==========================================
+ Hits          795      829      +34
+ Misses        191      171      -20

see 3 files with indirect coverage changes


from-source feature on macOS: Assertion failed: (var == vars[j]), function presolRoundCardinality, file cons_cardinality.c, line 932.

Running this on my Macbook Pro M3 with russcip v0.3.3's new feature from-source:
❯ cargo install russcip -F from-source --examples
    Updating crates.io index
  Installing russcip v0.3.3
    Updating crates.io index
   Compiling libc v0.2.153
   [...]
   Compiling russcip v0.3.3
    Finished release [optimized] target(s) in 2m 30s
  Installing /Users/jacob/.cargo/bin/create_and_solve
   Installed package `russcip v0.3.3` (executable `create_and_solve`)

I get this error:
❯ create_and_solve
dyld[26197]: Library not loaded: libscip.9.0.dylib
  Referenced from: <C3392B20-78D6-3592-8F6D-77169937FA93> /Users/jacob/.cargo/bin/create_and_solve
  Reason: tried: 'libscip.9.0.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OSlibscip.9.0.dylib' (no such file), 'libscip.9.0.dylib' (no such file), '/Users/jacob/libscip.9.0.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/Users/jacob/libscip.9.0.dylib' (no such file), '/Users/jacob/libscip.9.0.dylib' (no such file)
[1]    26197 abort      create_and_solve

Note to self: brew install bison was needed to get compilation to work

Sorry but I see now that I pasted the wrong error. I will close this issue for now and try to reproduce the error I originally received. If I'm able to I'll re-open.


Add GreenSock-JS globals?

Hi I'm currently working with GreenSock-JS (https://github.com/greensock/GreenSock-JS) and they have a bunch of globals (http://greensock.com/forums/topic/11369-gsap-globals-for-jshint/)
"globals": {
        //GSAP Globals
        "TimelineLite" : false,
        "TimelineMax" : false,
        "TweenLite" : false,
        "TweenMax" : false,
        "Back" : false,
        "Bounce" : false,
        "Circ" : false,
        "Cubic" : false,
        "Ease" : false,
        "EaseLookup" : false,
        "Elastic" : false,
        "Expo" : false,
        "Linear" : false,
        "Power0" : false,
        "Power1" : false,
        "Power2" : false,
        "Power3" : false,
        "Power3" : false,
        "Power4" : false,
        "Quad" : false,
        "Quart" : false,
        "Quint" : false,
        "RoughEase" : false,
        "Sine" : false,
        "SlowMo" : false,
        "SteppedEase" : false,
        "Strong" : false,
        "Draggable" : false,
        "SplitText" : false,
        "VelocityTracker" : false,
        "CSSPlugin" : false,
        "ThrowPropsPlugin" : false,
        "BezierPlugin" : false
}

I wish I could just write
GSAP: true

Accepting library-specific globals is a slippery slope of supporting all sorts of lesser-used libraries. Instead of trying to define which libraries are "popular enough" to justify inclusion, we're reserving new environment settings for true environments.
The GreenSock library might consider exporting a single variable on the global scope and using that as a namespace for its API. I only suggest changing the source code (instead of JSHint) in this case because the change would do more than address this specific problem (making linting configuration easier to manage). It's generally considered a best practice because it minimizes risk of collisions and it requires consuming code to be easier to follow (the origin of every method is communicated each time it is accessed).

I can't agree more :D

Thanks for understanding!


ogen: init at 1.3.0

Description of changes

add ogen package - https://github.com/ogen-go/ogen

OpenAPI v3 Code Generator for Go.

release notes for v1.3.0 - https://github.com/ogen-go/ogen/releases/tag/v1.3.0
note that I added a patch which hardcodes the version number - this is because this tool takes the version info from git via the go build system - if the source is obtained using fetchFromGithub, the build does not take place inside a git repo and hence no version info can be inferred

would be particularly interested in thoughts on whether this is a good/sensible approach

Also note that #299164 did focus on this but I closed it as @dz0ny indicated that he could not progress it.
Things done

Built on platform(s)

[x] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[x] Tested basic functionality of all binary files (usually in ./result/bin/)
24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[x] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/prs-ready-for-review/3032/4558


support global commands in get_bgp_config()

This provides support for BGP commands applied globally as they are currently ignored. Now, I don't know how to return this data, I just included a new key called GLOBAL, but not sure if there is a better approach
{
    "GLOBAL": {
        "apply_groups": [],
        "description": "",
        "export_policy": "p1",
        "import_policy": "p2",
        "local_address": "",
        "local_as": 1111,
        "multihop_ttl": 64,
        "prefix_limit": {},
        "remove_private_as": true
    },
    "internal": {
        "apply_groups": [],
        "cluster": null,
        "description": "",

This is breaking the tests using the mocked data.... but before fixing that it would be good to know if this PR is approved or not. Thanks

First step to modify an existing method data structure is to open an issue/PR in napalm-base to discuss the topic. However, as we are going to go into a code freeze soon for the "reunification" process I'd suggest discussing this after it's complete.

@dbarrosop: thanks, we can discuss that once the "reunificaiton" is over. Closing


[$500] Settings – Cleared status is visible if press save button

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

Version Number: 1.4.19-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:

Go to https://staging.new.expensify.com/
Log in with any account.
Open Settings/ Profile/ Status select a any emoji
Verify that status emoji appears
Clear status and verify that status emoji cleared
Press Save button

Expected Result:
Cleared status is not visible if press save button
Actual Result:
Cleared status is visible if press save button
Workaround:
Unknown
Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[ ] iOS: mWeb Safari
[x] Windows: Chrome
[ ] MacOS: Desktop

Screenshots/Videos
Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/7b7a13dc-b128-4682-80df-5d1acbf1fe55
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0166fc8d8a8bf48267
Upwork Job ID:<PHONE_NUMBER>054357504
Last Price Increase: 2023-12-29

Proposal
from: @unicorndev-727
Please re-state the problem that we are trying to solve in this issue.
Additional edit system messages are shown when request is created offline with edits
What is the root cause of that problem?
The root cause is that we set the default emoji to this one here.
formRef.current.resetForm({[INPUT_IDS.EMOJI_CODE]: initialEmoji});

What changes do you think we should make in order to solve the problem?
We need to update this line
const initialEmoji = '💬';

to
const initialEmoji = '';

My solution same like @unicorndev-727

Proposal
Please re-state the problem that we are trying to solve in this issue.
Default status has been set when we click on Save after clearing it
What is the root cause of that problem?
When we click on Save button we reset the status and emoji code. So when we click save post clearing the status it considers the default emoji code as a new status and saves it.
What changes do you think we should make in order to solve the problem?
We should navigate to the previous screen when the user press Clear Status bcoz it doesn't depend on the Save button to apply its changes.

Result
https://github.com/Expensify/App/assets/14358475/fce2112a-89b1-4b77-9edb-d04c3aeb9123

Proposal
Please re-state the problem that we are trying to solve in this issue.
Settings – Cleared status is visible if press save button
What is the root cause of that problem?
UX issue. After status is cleared, user think the save button is for saved the clear action.
What changes do you think we should make in order to solve the problem?
Hide the "Save" button when user click on "Clear status".
Keep the button hidden using isSubmitButtonVisible until the user change the value of the emoji on the status page emojiPicker..
https://github.com/Expensify/App/blob/320ff544570be797b018279693fcd44da8b838c7/src/pages/settings/Profile/CustomStatus/StatusPage.js#L140-L149

@Ollyws the proposal by dukenv0307 will not work when the user pick this icon 💬 from the emojiPicker

it's used as default icons too, so the condition is always true.

I think we try to avoid conditionally showing/hiding UI when possible. So that being said, part of me thinks we should just show "Clear status" at all times, the same way we always show a form submit button at all times. Curious what the @Expensify/design thinks about that.
Otherwise let's just fix the issue and not flash the "Clear status" row when you hit the save button.

I think if the user hits Save and the status is empty, we just save the empty status and head back to the previous page.

I don't mind that, though I suppose you might be clearing your status so that you can change it for a new one right?

I think if the user hits Save and the status is empty, we just save the empty status and head back to the previous page.

The problem is when we "clear" the status we're not setting it to be empty in the form (even though we're setting it to be empty for the app). In the form, we're setting it to be "💬". So there's no possible way to save an empty status.

Actually clear status button already resets the status, so I suggested in my proposal to navigate back when user presses clear status.

I think I'm in favor of that solution. I agree that you might be clearing your status to set a new one... but you can also just change it and press save without clearing it first. Navigating away feels like the simplest/cleanest/most clear solution to me.

The navigation back is good, but what when the user turn back again to the Status screen but click save without picking any Emoji? It's the same issues.
Keeping the Save button hidden will solve this issue tto, even when user back for the 2nd time, nho way to pick a wrong emoji

I think it is more clear to remove status emoji in emoji selector when clicking clear button so that user can confirm that he cleared emoji and then select new one or save empty status.

Yeah, I think I agree - clear is just to clear the form, but you still need to explicitly save it.

In that case I think we need to show some placeholder emoji icon instead of the one from the list

Yup, I think this got lost in translation somewhere but we should be using our emoji icon as the placeholder... not an actual emoji. @perunt any idea what happened there? Something like this:

I assumed Clear Status is the feature to clear it directly for the user, not just reset the form for them.

Cool, I can get down with Clear actually clearing the form AND saving. So to recap, my totally non-official stance is:

Never conditionally show/hide the Save button. Just show it at all times
Use our emoji icon as the placeholder for the emoji picker
Clear button clears the status and saves, but does not navigate back to previous page. This way you can clear your status and quickly add a new one. I feel the least strongly about this point though, I can see an argument that clear should navigate back to prior page.
Save button should save and navigate back to previous page, like it does on other forms (saving profile details for instance or any push-to-page input).

Agree! With the caveat that I actually think the clear button should navigate away to mirror the behavior of the save button. You don't have to clear a status before adding a new one, you can just change it and save it. But like you, I feel the least strongly about this point.
Everything else, 100% for it.

Cool cool, maybe we can have @dubielzyk-expensify make the decision on that one then!

If the user only enters a status text and no emoji, we could:

use a default message emoji
or return a form error and request that they pick an emoji

I don't feel too strongly, curious for @Expensify/design thoughts

I lean towards default emoji 🤔

Slack does default emoji as soon as you start typing a text status. That seems fine to me. There'd essentially be no way to see someone's status if they didn't select an emoji, so it makes sense to require it. I would rather just pick the default than throw an error though because I think setting a status should always be as fast and seamless as possible.

But I actually don't think we should navigate away due to the assumption being that they'll clear in order to change it. It feels weird that it tosses you out and you'll have to navigate back in. I don't feel super strongly though.

I kinda think this is a false assumption (but this is my own personal assumption lol). I have never cleared my status to set a new one. I just update and save. I'll also note that clearing a status in Slack boots you out of the status modal the same way saving a new status does. Having said that, while I totally disagree with y'all 😆 I'm down for that behavior if you both think it's best. How do we let people know that they don't need to click "Save" after they clear their status though? I feel like that's still an issue if we don't navigate away.
https://github.com/Expensify/App/assets/7516624/075499f2-684c-4582-bf21-3357785f6714

Hmm that's a really great point... navigating away after clear kinda indicates the Save action to the user, which does feel more clear. I think I am on team Danny now 🤣

Good points. I still don't like the auto-navigation, but maybe what I'm getting is that there's a different way of doing this UI in general, but that's a can of worms. 🤔
I love it over at Team Danny so I'll join ❤ 😄

Great discussion to catch up on.
I am on team Danny and navigating away as well. It feels the most clear to me and it's what I'm already used to with Slack so it feels comfortable.

🤗

I still don't like the auto-navigation, but maybe what I'm getting is that there's a different way of doing this UI in general, but that's a can of worms.
I agree that there's likely a better, different way of doing this UI, but like you I'm wary of the worms 😆.
Even though I feel like we have a path forward, I'm still curious what you don't like about the auto-navigation? It works the same as the save behavior—is that an issue for you as well?

I'm still curious what you don't like about the auto-navigation?

It's such a subjective feeling. The save button is a footer button and feels more attached to the "chrome" of the view, but the clear button is located next to the input field, so it feels like I'm gonna impact that rather than navigate away.
Anyways, I think we got consensus and I don't feel strongly. I think you've made convincing arguments for why it's better 😄

The save button is a footer button and feels more attached to the "chrome" of the view, but the clear button is located next to the input field, so it feels like I'm gonna impact that rather than navigate away.

I think this is totally sound reasoning. In fact, I almost suggested earlier that we move the clear button to be down in the footer along with save because I think I was feeling that same tension but didn't know how to verbalize it 😂
Anyhoo, let's move forward, but I think we (@Expensify/design) can keep this in mind as something that can be improved.

@dannymcclain - Do you have an opinion on which wave/vip this should belong to?

@strepanier03 😬 I do not. Do you? Or does anyone? This feature feels specifically/heavily related to chat, so do we have any other waves/vips where we're prioritizing chat features?

Got it, that helps. I'll check the most common waves/VIPs for chat and see who snags it.

Posted here to see if including in VIP-VSB is the correct step.

All right, confirmed the VIP to include this and updated that.

What's the update here?  Should we pick a different proposal?

All right, everyone's been paid and contracts closed. Thank you all!


Control actions

Implement support for all flow actions. Flow actions are the actions which is used in almost every flow, the control the flow.
The flow actions are:

[x] Condition
[x] Apply to each
[x] Do until
[x] Scope
[x] Switch
[x] Terminate

:tada: This issue has been resolved in version 1.0.0-alpha.22 :tada:
The release is available on:

GitHub release
v1.0.0-alpha.22
v1.0.0-alpha.22

Your semantic-release bot :package::rocket:

:tada: This issue has been resolved in version 1.0.0-alpha.22 :tada:
The release is available on:

GitHub release
v1.0.0-alpha.22
v1.0.0-alpha.22

Your semantic-release bot :package::rocket:

:tada: This issue has been resolved in version 1.0.0 :tada:
The release is available on:

GitHub release
v1.0.0
v1.0.0

Your semantic-release bot :package::rocket:

:tada: This issue has been resolved in version 1.0.0 :tada:
The release is available on:

GitHub release
v1.0.0
v1.0.0

Your semantic-release bot :package::rocket:


Defining modules of type other than csi panics with panic: runtime error: invalid memory address or nil pointer dereference

Summary
Defining modules of type other than CSI will panic
rexray:
  modules:
    gp2-100gib:
      type: docker

Bug Reports
Version
REX-Ray
-------
Binary: /usr/bin/rexray
Flavor: client+agent+controller
SemVer: 0.11.3
OsArch: Linux-x86_64
Commit: 3afc7942dd8b1a88ed902344b4ece89374be87f7
Formed: Tue, 12 Jun 2018 15:45:51 UTC

Expected Behavior
Not to panic, or at least to say unsupported module and to exit.
Actual Behavior
DEBU[0002] found cached instance ID                      host=unix:///var/run/rexray/575787139.sock integrationDriver=linux osDriver=linux server=daisy-hound-sc service=ebs storageDriver=libstorage time=1534351173145 txCR=1534351173 txID=46f82548-c8eb-442b-428f-14f07c4312cd
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x194386b]

goroutine 220 [running]:
github.com/rexray/rexray/agent/csi/libstorage.(*driver).Serve(0xc420283000, 0x336b200, 0xc420052180, 0x7f7006cfdae0, 0xc4200c9080, 0x0, 0x336b200)
	/go/src/github.com/rexray/rexray/agent/csi/libstorage/csi_service_libstorage.go:107 +0x4cb
github.com/rexray/rexray/agent/csi.(*csiService).Serve(0xc420285090, 0x336b200, 0xc420052180, 0x0, 0x0, 0x0, 0x0)
	/go/src/github.com/rexray/rexray/agent/csi/mod_csi_service.go:72 +0x65
github.com/rexray/rexray/agent/csi.(*mod).Start.func2.1(0xc42041be00, 0x33744a0, 0xc420052180)
	/go/src/github.com/rexray/rexray/agent/csi/mod_csi.go:247 +0x8a
created by github.com/rexray/rexray/agent/csi.(*mod).Start.func2
	/go/src/github.com/rexray/rexray/agent/csi/mod_csi.go:246 +0x57

Steps To Reproduce

Add any module definition of type not csi

  modules:
    gp2-100gib:
      type: docker

Configuration Files
rexray:
  logLevel: debug
  modules:
    default-csi:
      desc: "The default CSI module."
      disabled: true
    gp2-100gib:
      logging:
        level: info
      service: ebs
      type: docker
      disabled: false
      desc: 100GiB General Purpose
      # host: /run/docker/plugins/rexray.sock
      host: unix:///run/docker/plugins/gp2-100gib.sock
      spec: /etc/docker/plugins/gp2-100gib.spec
      libstorage:
        service: ebs
        integration:
          volume:
            operations:
              create:
                default:
                  type: gp2
                  size: 100
                  fstype: xfs
libstorage:
  logging:
    level: debug
  service: ebs
  server:
    tasks:
      exeTimeout: 120s
    services:
      ebs:
        driver: ebs
  integration:
    volume:
      operations:
        create:
          default:
            size: 5
        mount:
          preempt: true
ebs:
  accessKey: AWSACCESSKEY
  secretKey: AWSSECRETKEY
  region: AWSREGION

Opinion
Attempting to retrieve the configuration szTimeout := d.config.GetString("csi.libstorage.timeout") will panic as the code
// Check for a gofig.Config in the context.
if config, ok := d.ctx.Value(ctxConfigKey).(gofig.Config); ok {
   	d.config = config
   	d.mntPath = d.config.GetString(apitypes.ConfigIgVolOpsMountPath)
}

will never be ok, as the key ctxConfigKey defined as ctxConfigKey     = interface{}("csi.config") is not there.
The code should not panic and if it does not support other module types, should either not have this field or provide the user with an appropriate error.

Hello @svyotov,
What are you trying to achieve with your configuration?

Hi @clintkitson we were trying to configure volume sizes and other options. CSI will work fine as far as I can see, the issue though was that when providing a different type: docker, which was what version 0.9.0 would work with, it panics. It would be better if it would handle such issues more gracefully and provide with information on what configurations are not valid.


permissions wont take affect?

I set my perms correctly and they even show up on the web editor. i have 3 groups and they're all connected. i have default -> mod -> owner. All of the perms though won't work for even the default group. what is wrong? I'm using LuckPerms 4.4.1

Have you tried debuggig with Verbose?

Have you tried debuggig with Verbose?

how would i debug with it? i tried on and it just spammed chat

Did you read further on into the page? It explains about filters which help avoid the spam.

yeah i tried to look into the filters. and it only says I (i'm OP) have the /help but in the web editor it said everyone had it

Never debug permissions with OP, of course it's going to tell you that you have everything.
De-op yourself, put yourself in the group you are trying to debug and try again.

okay I de-oped myself and i ran the command in the console and now it shows no one has the commands i filtered

So it is saying undefined next to the permissions? Please be more specific.
If so, that means you have not set the permission.

in the webeditor it says i did. what do i do? should i remove all the perms and redo them?

Post your web editor link, and share a screenshot of what verbose shows.

https://luckperms.net/editor/#8OPajRlzeH and

and this when i tried /help

You haven't added any permissions correctly, you have just added the end of each node, you need to use the FULL permission not just the end of it.

oh hell okay


feat: add function for commanders to stop execution

idea: pass some kind of self arg to commanders which provides an exit command. this is kind of an alternative to a return with exit code
(would be a breaking change - but shouldnt be one)
example:
local commander = require 'commander'

commander.register('test', function(self, args)
    self:exit(2)
end)

This would probably not work properly unless the function is somehow run in a seperate thread. Also the example provided is completely incompatible with the current function(args, sink) currently used. It might be better to make sending an EOF to sink.out stop monitoring the function, or make a sink closable

i don't remember why i thought of this in the first place, so i think i'm just gonna close.


Diconnect players

detect if players closed the game and then kill them.
free the id so others can join the game.

https://github.com/chichilku/chichilku3/commit/cf39650994cbd1986c520948de7f0a88d4487e71


Add static analysis

Please start CI :)

@DarkGhostHunter Please consider running static analysis.
https://github.com/szepeviktor/Laraconfig/runs/3106068821?check_suite_focus=true 👀

What when why how?

As in this PR: https://github.com/DarkGhostHunter/Laraconfig/pull/4/files

Install Larastan
Run it :)

The above CI output shows some problems/typos/false positives.

Thanks but I'm not interested in static analysis... yet (specially when on many false positives and magic methods).


Enforce max capacity for AlignedVec

AlignedVec's implementation uses the unsafe function alloc::Layout::from_size_align_unchecked() in many places. The requirements in terms of alignment are trivially met, but there is an odd third restriction:

size, when rounded up to the nearest multiple of align, must not overflow isize (i.e., the rounded value must be less than or equal to isize::MAX).

As the alignment of AlignedVec is 16, this implies the maximum capacity needs to be isize::MAX - 15. Currently the code allows capacity of up to usize::MAX - roughly twice as large.
It's therefore trivial to trigger UB:
AlignedVec::with_capacity(usize::MAX)

This PR:

Enforces the correct maximum capacity.
Fixes a potential arithmetic overflow in reserve().

In practice, I'm not sure if it's feasible to exceed the maximum anyway, as you'd run out of memory first, at least on 64-bit systems. It's probably possible on 32-bit systems (trying to use over 2 GB of the 4 GB available). But I don't think these changes add any overhead for the hot paths, so there seems little harm in playing it "by the book".
When capacity exceeds isize::MAX / 2, rather than increasing by power of 2 (which would exceed max), it caps the capacity at the maximum allowable. This is to allow up to 2 GB capacity on 32-bit systems (rather than 1 GB otherwise).
This PR builds on top of #348, because they touch the same code.

Thanks for merging this so swiftly.

Thanks for the PR!


ion-datetime bug in min max range

Ionic version:  (check one with "x")
[ ] 1.x (For Ionic 1.x issues, please use https://github.com/ionic-team/ionic-v1)
[ ] 2.x
[X] 3.x
I'm submitting a ...  (check one with "x")
[X] bug report
[ ] feature request
[ ] support request => Please do not submit support requests here, use one of these channels: https://forum.ionicframework.com/ or http://ionicworldwide.herokuapp.com/
Current behavior:
Guys, it occurs in the display of the date picker:
I set hardcoded to an specific date like (2017-10-10) and it always show the current day of operating system ignoring what I had set. Month and year shows okay but the day always appears wrong.
Expected behavior:
Get the day corretcly.
Cordova CLI: 7.0.0
Ionic Framework Version: 3.2.0
Ionic CLI Version: 2.2.1
Ionic App Lib Version: 2.2.0
Ionic App Scripts Version: 1.3.6
ios-deploy version: Not installed
ios-sim version: Not installed
OS: Linux 4.4
Node Version: v6.9.3
Xcode version: Not installed

Hello, thanks for opening an issue with us. I am going to close this issue as a duplicate of https://github.com/ionic-team/ionic/issues/12070


fluxcd: init at 0.5.0

Motivation for this change
Add fluxcd
This is flux v2. There is also fluxctl (flux v1). v1 is in maintenance
mode, and it will be deprecated at some point, but some people may still
depend on v1.
Things done

[x] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
Built on platform(s)

[x] NixOS
[ ] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
[x] Tested execution of all binary files (usually in ./result/bin/)
[ ] Determined the impact on package closure size (by running nix path-info -S before and after)
[ ] Ensured that relevant documentation is up to date
[x] Fits CONTRIBUTING.md.

/marvin opt-in
/status needs_reviewer

Result of nixpkgs-review pr 106483 1

1 package built:

fluxcd

/status needs_merger