Tyrsis / SE-Community-Mod-API

Space Engineers Community Modding API
GNU Lesser General Public License v3.0
23 stars 11 forks source link

Chat related plugins arent working. #45

Open Vl4dim1r opened 9 years ago

Vl4dim1r commented 9 years ago

Hey Tyrsis, I decided to open an issue here because it seems to be bigger than essentials. As i said in that issue, non of Essentials chat commands do anything. Now we have discovered that our custom chat relay isnt sending text back since thr latest release of SESE. There are no errors anywhere, but no results either.

McyD commented 9 years ago

Actually there is an error, I posted it in another issue, but as this directly relates to it, reposting it here.

2015-01-29 17:46:12.311 - Thread: 1 -> Log Started 2015-01-29 17:46:12.311 - Thread: 1 -> Timezone (local - UTC): -5h 2015-01-29 17:46:12.311 - Thread: 1 -> App Version: 0.3.0.1 2015-01-29 17:46:12.312 - Thread: 1 -> Failed to find method '582447224E2B03FA4EAB3D6C2DDD48D9' in type '5F381EA9388E0A32A8C817841E192BE8.0F8EE1AD651CB822CB9635B463AE6CD5' 2015-01-29 17:54:10.690 - Thread: 11 -> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.MissingFieldException: Field not found: 'ChatEvent.message'. at EssentialsPlugin.Essentials.OnChatReceived(ChatEvent obj) --- End of inner exception stack trace --- at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor) at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments) at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters) at SEModAPIExtensions.API.PluginManager.DoUpdate(Object args) 2015-01-29 17:54:10.690 - Thread: 11 -> System.MissingFieldException: Field not found: 'ChatEvent.message'. at EssentialsPlugin.Essentials.OnChatReceived(ChatEvent obj)

dodexahedron commented 9 years ago

This is most likely related to the existing issues already reported. I'll be releasing a version of Essentials that is compatible with the current version of SESE shortly.

dodexahedron commented 9 years ago

Please download this release: https://github.com/dodexahedron/EssentialsPlugin/releases/tag/v1.0.2.1669-alpha

The Essentials release currently available on this fork is only compatible with versions of SESE before 0.3.0. That will be fixed soon, but until then you can download from my fork.

Vl4dim1r commented 9 years ago

What do I need to do for my chat relay to get messages again?

Sorry I didnt see the error, I'm at work and having my proxy admin relay everything, he didnt check the internal log. On Jan 29, 2015 5:55 PM, "dodexahedron" notifications@github.com wrote:

Please download this release: https://github.com/dodexahedron/EssentialsPlugin/releases/tag/v1.0.2.1669-alpha

The Essentials release currently available on this fork is only compatible with versions of SESE before 0.3.0. That will be fixed soon, but until then you can download from my fork.

— Reply to this email directly or view it on GitHub https://github.com/Tyrsis/SE-Community-Mod-API/issues/45#issuecomment-72129012 .

Vl4dim1r commented 9 years ago

Here is one of the errors

2015-01-29 18:23:46.337 - Thread: 19 -> System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.MissingFieldException: Field not found: 'ChatEvent.sourceUserId'. at EttnaWebRelay.Relay.OnChatReceived(ChatEvent chatEvent)

Moustachauve commented 9 years ago

dodexahdron broke all existing plugin because he updated some critical variable, like "sourceUserId" becoming "SourceUserId". You should avoid doing this in the future, as this force plugin maker to update their plugin just for a simple and stupid naming convention

rogeriocamorim commented 9 years ago

Sorry made, but this is not a simple and stupid name change. What he did is the right thing to do. You need to create rules for method names and variables. All name from method should start with uppercase letter. This is a good programming practice. Em 29/01/2015 23:02, "Christophe Gagnier" notifications@github.com escreveu:

dodexahdron broke all existing plugin because he updated some critical variable, like "sourceUserId" becoming "SourceUserId". You should avoid doing this in the future, as this force plugin maker to update their plugin just for a simple and stupid naming convention

— Reply to this email directly or view it on GitHub https://github.com/Tyrsis/SE-Community-Mod-API/issues/45#issuecomment-72136154 .

Moustachauve commented 9 years ago

It is indeed a good programming practice, but not in this case. I think breaking plugin must be avoided at all cost

Moustachauve commented 9 years ago

Plus this project already had a written coding convention, which he just screwed all over

https://github.com/SEModCommunity/SE-Community-Mod-API/wiki/Coding-Standard-reference

rogeriocamorim commented 9 years ago

Just fix the method. It is easy. Son they will updated it. Em 29/01/2015 23:50, "Christophe Gagnier" notifications@github.com escreveu:

It is indeed a good programming practice, but not in this case. I think breaking plugin must be avoided at all cost

— Reply to this email directly or view it on GitHub https://github.com/Tyrsis/SE-Community-Mod-API/issues/45#issuecomment-72140617 .

Moustachauve commented 9 years ago

It's not that easy, especially since he didn't even told anyone what changed. Even Tyrsis plugin is broken. It might be "easy" for me to fix, but while I'm fixing it (I don't have tons of free time to do it), the user can't use any plugin (Tyrsis plugin still needs to be updated...), so it is a really bad idea

Vl4dim1r commented 9 years ago

As Linus would put it: "DON'T F***Ing BREAK USERSPACE" On Jan 29, 2015 7:25 PM, "rogeriocamorim" notifications@github.com wrote:

Sorry made, but this is not a simple and stupid name change. What he did is the right thing to do. You need to create rules for method names and variables. All name from method should start with uppercase letter. This is a good programming practice. Em 29/01/2015 23:02, "Christophe Gagnier" notifications@github.com escreveu:

dodexahdron broke all existing plugin because he updated some critical variable, like "sourceUserId" becoming "SourceUserId". You should avoid doing this in the future, as this force plugin maker to update their plugin just for a simple and stupid naming convention

— Reply to this email directly or view it on GitHub < https://github.com/Tyrsis/SE-Community-Mod-API/issues/45#issuecomment-72136154>

.

— Reply to this email directly or view it on GitHub https://github.com/Tyrsis/SE-Community-Mod-API/issues/45#issuecomment-72138288 .

McyD commented 9 years ago

I should point out, that as far as most are aware, there is 2-3 plugins for SESE... According to Tyrsis, if you look through the project you can see he asked before making the changes, as he knew some changes could break things.

Maybe before griping about something is broken, you should try and promote that you have something that uses it. And I am also positive I see the name ALPHA on the project downloads.

So before jumping all over the person, who I have seen (besides Tyrsis) give more to the project in the last few days than anyone else has in months, maybe you should pitch in as well or at least stay up to date with the changes being made.

Moustachauve commented 9 years ago

Can you point me out where he asked before making the changes? I can't find it (If I can find it I don't think we can considere it really "asking")

only 2-3 plugins? I'm maintaining some private plugins, I'm in the operation of creating my own plugin, I'd like to not have a ton of work every update, I don't have time for that

Give more work? he only renamed thing, that added absolutely no value to the project. He lost his time. performance gain? Read only are painfully slow...

Tyrsis commented 9 years ago

Luckily you can fork the original, and change it all back to how you want. You are not forced to follow my fork. I agreed with a good portion of his updates. While I may not completely agree with the formatting, it was so incredibly tiny. All he did to some things were capitalize where it should have been (the chat event class was all small case for some reason, where as everything else followed normal procedure).

The updates take seconds, and you can recompile as you see fit. There is not enough usage of extender in the plugin sense for me to be concerned about compatibility right now. This answer may make you feel uncomfortable, but I have to recompile my stuff multiple times a week as it is. It's just how it goes. It was the first person in at least 3 months who has offered any additional help on this project, and actually dedicated some time to it.

McyD commented 9 years ago

@Moustachauve

Try reading the pulls, start with https://github.com/Tyrsis/EssentialsPlugin/pull/72. He announced the changes, asked if he should avoid anything, and checked with the only other major contributor.

Moustachauve commented 9 years ago

@McyD Why should I need to go through the pull request to know that variable names changed? it should be stated in changelog. which was not the case.

@Tyrsis I get that, but still I don't get why changing the convention was needed, or if anything he did added any value to the project

Tyrsis commented 9 years ago

He actually reorganized chat in a way I wanted to. Compare the chatmanager, it cleaned it up a lot. The other changes he did were small. While I may not completely agree with parenthesis spacing, there were other small changes he did that actually made things more readable, so I merged them. It is something that can be discussed further and changed if it's too much of an issue, but I see nothing wrong with it, and welcome the help.

dodexahedron commented 9 years ago

Yeah the parentheses spacing I somewhat apologize on (sorta) :tongue: It's just the way I've always had visual studio set and, when it auto-formats, it changes that. So, rather than manually undoing that, I just let it do it. If people don't like it, just press ctrl + e, d and it'll revert to your local settings. That's why I kinda didn't care on that one.

As for the rest of the structural changes, I have no apologies. The original project flew in the face of typical C# practices, and was kinda hard to deal with. Go here to get an idea of what is typical: https://msdn.microsoft.com/en-us/library/ms229002%28v=vs.110%29.aspx

In any case, this is an alpha, and you have no reason to expect it to be consistent from one release to the next. If you build things that are dependent on it, it's YOUR responsibility to be sure you keep up to date with master before you install the latest version. The alternative is just don't update or else fork the version you want yourself, as Tyrsis mentioned.

Alpha is precisely the right time to fix all these minor issues, before they become well-baked into a project that is no doubt going to be a relatively important one, going forward. Once we slap a "beta" tag on it, things need to start staying a lot more consistent from release to release, but until then just pay attention and don't update without reading the release notes and the code diffs. And CERTAINLY don't deploy it to your server without trying to compile your own code against it, first. That's just asking for trouble, no matter what production level tag is assigned to it, and that's true of ANY software, in ANY situation.

I empathize with your pain of having to adapt to changes that you can't control, but it's just a fact of life when dealing with alpha software. The best I can do is assure you that it'll slow down once we get it organized better and that the majority of changes I make that are to public members will be naming-only.

joshuataylor commented 9 years ago

Exactly, this is ALPHA software, which means things will break.

Just be happy they don't break things more often, IMHO - look at other major projects.

Maybe what needs to happen is release a change records, similar to what other projects do for when they change something that will exist existing contributed modules/plugins?

As said above, even during BETA this may happen, but should never ever happen once 1.0 has hit.

Oh and it's also a usecase for having tests in code, or let your compiler pick them up :).

Moustachauve commented 9 years ago

I think this project will stay forever in alpha, from what I've seen, it's only maintained now, so that argument is useless. Fork it myself? the userbase is using this one...but yeah. Change record? I though tyrsis was doing one, but seems like no

joshuataylor commented 9 years ago

I believe it should leave Alpha should be as soon as SE leaves early access.

But yeah, forks are just silly as they split the community, we should instead have change records to alert people when something is going to break.

Create CHANGELOG with the changes, maybe UPGRADE for upgrading (this should be between major releases though)?

dodexahedron commented 9 years ago

@joshuataylor, yes, that feels like a reasonable timeline. It's unrealistic to expect this software to not be alpha while the software it explicitly depends on is still alpha. The timeline for releases on this will always slightly lag SE, so I imagine we'd probably call SESE beta shortly after SE itself goes beta.

I responded to the changelog comment on the other issue you opened up explicitly for it.