Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dev Module #4087

Open
wants to merge 19 commits into
base: 1.21.1
Choose a base branch
from
Open

Add Dev Module #4087

wants to merge 19 commits into from

Conversation

IThundxr
Copy link
Contributor

Also adds properties for enabling certain things controlled by SharedConstants.isDevelopment so developers can pick and choose what they want to enable instead of everything being enabled

@modmuss50 modmuss50 added the enhancement New feature or request label Sep 12, 2024
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Please make sure it also builds.

fabric-api-dev/README.md Outdated Show resolved Hide resolved
fabric-api-dev/src/main/resources/fabric.mod.json Outdated Show resolved Hide resolved
gradle.properties Outdated Show resolved Hide resolved
fabric-api-dev/README.md Outdated Show resolved Hide resolved
fabric-api-dev/README.md Outdated Show resolved Hide resolved
fabric-api-dev/README.md Outdated Show resolved Hide resolved
fabric-api-dev/README.md Outdated Show resolved Hide resolved
fabric-api-dev/README.md Outdated Show resolved Hide resolved
fabric-api-dev/README.md Outdated Show resolved Hide resolved
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

None of these should be enabled by default imo.

@@ -14,6 +14,7 @@ include 'fabric-api-bom'
include 'fabric-api-catalog'

include 'fabric-api-base'
include 'fabric-api-dev'
Copy link
Member

Choose a reason for hiding this comment

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

This should be set as a dev only module, and not included in the prod jar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason it's included in prod is since some developers requested that they would be able to use it outside of dev envs to be debugging in prod easier

* Default value: true</br>
* {@link CommandManager#execute(ParseResults, String)}
*/
public static final boolean ENABLE_COMMAND_EXCEPTION_LOGGING = getProperty("enableCommandExceptionLogging", true);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these should be named such as fabric.dev.command.logging.exception (with all of the others following the same pattern?

@modmuss50
Copy link
Member

Hi, sorry for not taking a look at this PR in a while, please don't heasiate to ask me to look at PRs its impossible for me to check them all. If you need to ask me multiple times please do.

I worry that very few people will opt into these features, most of whitch are some additional logging that may not always be useful to someone other than Mojang. Warnings that we log we need to be 100% sure that they are important otherwise developers will just start ingoring them all. If its bad enough that it should never happen we should crash the game instead.

We already enable the most useful features that are locked behind SharedConstants.isDevelopment by default in a number of related modules such as /test command, /debugconfig command, and some argument type debug commands.

The module name also doesnt feel right as well, I think it maybe should include api as it does technically have an API (the system props). I also strongly think this should be a dev only module, it can still be installed in prod if someone wishes to by downloading it from maven.

Before you spend anymore time on this lets discuss this and get input from others to see what they think, I think we need to strongly consider if this is in scope, adding a new sub-module has a none zero peformance cost let alone the maintaince this module will require on updates (its unlikely people are going to search for new usages of isDevelopment after an update).

@IThundxr
Copy link
Contributor Author

Hi, sorry for not taking a look at this PR in a while, please don't heasiate to ask me to look at PRs its impossible for me to check them all. If you need to ask me multiple times please do.

I worry that very few people will opt into these features, most of whitch are some additional logging that may not always be useful to someone other than Mojang. Warnings that we log we need to be 100% sure that they are important otherwise developers will just start ingoring them all. If its bad enough that it should never happen we should crash the game instead.

We already enable the most useful features that are locked behind SharedConstants.isDevelopment by default in a number of related modules such as /test command, /debugconfig command, and some argument type debug commands.

The module name also doesnt feel right as well, I think it maybe should include api as it does technically have an API (the system props). I also strongly think this should be a dev only module, it can still be installed in prod if someone wishes to by downloading it from maven.

Before you spend anymore time on this lets discuss this and get input from others to see what they think, I think we need to strongly consider if this is in scope, adding a new sub-module has a none zero peformance cost let alone the maintaince this module will require on updates (its unlikely people are going to search for new usages of isDevelopment after an update).

An alternative could be that we enable more things by default, this is gonna be a bit subjective, so it would require some conversations to see what exactly people feel should be enabled by default, but for a start i think the following are good:

  • zeroWeightWarning
  • logMissingTranslations
  • enableCommandExceptionLogging
  • enableCommandArgumentLogging
  • enableExceptionIdePausing

@modmuss50
Copy link
Member

An alternative could be that we enable more things by default, this is gonna be a bit subjective, so it would require some conversations to see what exactly people feel should be enabled by default, but for a start i think the following are good:

I dont think there is a perfect solution either way, some people want these additional checks enabled, others dont care. I think the better of the two evils is having them disabled by default. I do worry having too many warnings will make people ingore them all. Again this is likely out of scope but having a way for these warnings to crash the game might nice, this way a project such as Fabric API can ensure that the warnings never happen again.

enableExceptionIdePausing
This one I dont think thats useful for anyone (even Mojang tbh), just let the game crash, and if you want to breakpoint on an exception set a breakpoint on the exception. Id argue we shouldnt even have an option for this.

@Earthcomputer
Copy link
Contributor

Some of these should absolutely be enabled by default, such as enableCommandExceptionLogging.

Additionally, there are some other things that could be useful in the dev module that Mojang doesn't have, like fixing the server watchdog to not crash after being stopped at a breakpoint.

@modmuss50
Copy link
Member

modmuss50 commented Jan 14, 2025

Some of these should absolutely be enabled by default, such as enableCommandExceptionLogging.

This can be done in the command module.

like fixing the server watchdog to not crash after being stopped at a breakpoint.

I think this was discussed, and we agreed it was a good idea but couldnt come to a solid way to do it. Can be done in the crash reports module.

@IThundxr
Copy link
Contributor Author

An alternative could be that we enable more things by default, this is gonna be a bit subjective, so it would require some conversations to see what exactly people feel should be enabled by default, but for a start i think the following are good:

I dont think there is a perfect solution either way, some people want these additional checks enabled, others dont care. I think the better of the two evils is having them disabled by default. I do worry having too many warnings will make people ingore them all. Again this is likely out of scope but having a way for these warnings to crash the game might nice, this way a project such as Fabric API can ensure that the warnings never happen again.

enableExceptionIdePausing

This one I dont think thats useful for anyone (even Mojang tbh), just let the game crash, and if you want to breakpoint on an exception set a breakpoint on the exception. Id argue we shouldnt even have an option for this.

Making the warnings crash wouldn't be a bad idea

As for exception pausing it's useful as you can go down the stacktrace whenever a exception is thrown with it and see exactly what every variable or field is set to while the error was thrown

@modmuss50
Copy link
Member

As for exception pausing it's useful as you can go down the stacktrace whenever a exception is thrown with it and see exactly what every variable or field is set to while the error was thrown

But you can do this by setting a breakpoint on the exception its self, without needing to place a breakpoint in a weird location.

@IThundxr
Copy link
Contributor Author

As for exception pausing it's useful as you can go down the stacktrace whenever a exception is thrown with it and see exactly what every variable or field is set to while the error was thrown

But you can do this by setting a breakpoint on the exception its self, without needing to place a breakpoint in a weird location.

Sure, but now your needing to place a breakpoint at multiple different exceptions rather than just a single breakpoint in the Util class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants