refactor: replaced all whitelist and blacklist occurences by allowlis… #5277

byhlel · 2024-09-23T06:25:36Z

…t and blocklist

We want to demonstrate our openness and inclusiveness in our code

Resolves #5275

Contains

Resolves #5275

How to test

Everything works as before

Outstanding before merging

…t and blocklist We want to demonstrate our openness and inclusiveness in our code Resolves MovingBlocks#5275

Cervator · 2024-09-23T18:17:20Z

This is eerily reminding me of #5276 - my apologies @byhlel for thinking you may also be a bot or some sort of AI-assisted PR generator, but I'd like to ask for some sort of sign of humanity before considering this PR.

Like just saying hello and what brought you here, and confirming that you've actually been able to run the game from source with the updates in place?

byhlel · 2024-09-23T18:20:43Z

Hey,
I've just had some free time so I've been running around on good first issue.
I launched the game on a VM but im not on the best setup currently so I'd actually some review on this.
I'll try to rerun the game on another VM soon if no-one has time by then.

Also checking my profile would have reassured you on the latter.
Have a good one!

Cervator · 2024-09-23T18:27:05Z

Oh phiew then - thanks for the quick reply, just a weird timing issue with the other PR and the barebones PR description. Maybe a tool that partially auto-created the PR out of a commit message or something?

I'd also be curious how you ran it on a VM, when trying in the past here that has gone ... poorly :-)

Any VM-induced performance issues aside so long as it builds and runs that's probably enough to call it validated 👍

Although, considering that a rename of config files is included for a headless server run, that might be worth a quick check as well. And I wonder if we had any part of that automated for the test server setup / stored game server images we use 🤔

This shouldn't be too bad to try out though, so maybe myself or another maintainer can check that soon. I'm on parental leave and have pretty minimal time, just spotted this by chance. I'll try to keep this in a tab near the top of my browser and see if I can get to it :-)

Much appreciated!

byhlel · 2024-09-24T05:35:30Z

No I just like to keep my commits as concise as possible and respecting the repo's guideline.
So unfortunately (or fortunately) no tools neither to commit nor PR except from maybe lazyness to describe further the PR which was to me straightforward.

Seems fine to me but further review to make sure nothing was accidently broken might be relevant here.

BenjaminAmos

Thanks for doing this. We have sadly had pull requests that were very suspect in the recent past but they were clearly so (like #5264) and nothing like this. We are quite happy to accept quality contributions from anyone.

Ordinarily though, renaming APIs would be a breaking change that would need a strong justification to warrant large-scale changes to our module ecosystem.

The changes here, however, appear minimal and only affect internal APIs, so I see no harm in accepting it, so long as backwards compatiblity is maintained. Currently with these changes whitelist.json and blacklist.json will no longer be read. If you could change this to accept both old and file names still then it should work well.

BenjaminAmos · 2024-09-25T17:46:41Z

.gitignore

-# Ignore Whitelist and Blacklist files
-whitelist.json
-blacklist.json
+# Ignore Allowlist and Denylist files
+allowlist.json
+denylist.json


Can we please retain the old flle names in the gitignore so that they are not accidentally committed from older workspaces in future?

BenjaminAmos · 2024-09-25T17:47:05Z

docs/Modding-API.md

@@ -1,13 +1,13 @@
 Modding API
 =================

-Terasology's engine uses a whitelisting approach to expose an API for modules using two primary methods and a rarely needed third one:
+Terasology's engine uses allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:


Suggested change

Terasology's engine uses allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:

Terasology's engine uses an allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:

Or alternatively:

Suggested change

Terasology's engine uses allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:

Terasology's engine uses allowlisting to expose an API for modules using two primary methods and a rarely needed third one:

BenjaminAmos · 2024-09-25T17:49:36Z

engine/src/main/java/org/terasology/engine/network/internal/ServerConnectListManager.java

-        blacklistPath = PathManager.getInstance().getHomePath().resolve("blacklist.json");
-        whitelistPath = PathManager.getInstance().getHomePath().resolve("whitelist.json");
+        denylistPath = PathManager.getInstance().getHomePath().resolve("denylist.json");
+        allowlistPath = PathManager.getInstance().getHomePath().resolve("allowlist.json");


Since this is likely to a breaking change otherwise, can we continue to check for the old file names if the newer ones do not exist?

engine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/SelectGameScreen.java

…bility Code strucutre is different in SelectGameScreen and ServerConnectListManager due to compiling error otherwise. We check if path exist then we set the value if it does, otherwise it applies "previous" behaviour (meaning in case if even blacklist or whitelist) do not exist it sets the file to null. So no different behaviour is to be expected.

byhlel · 2024-09-26T04:54:04Z

Yes thank you for pointing this out, hadn't thought much of backwards compatibility!

BenjaminAmos · 2024-09-29T18:15:12Z

engine/src/main/java/org/terasology/engine/rendering/nui/layers/mainMenu/SelectGameScreen.java

-            Path blacklistPath = PathManager.getInstance().getHomePath().resolve("blacklist.json");
-            Path whitelistPath = PathManager.getInstance().getHomePath().resolve("whitelist.json");
-            if (!Files.exists(blacklistPath)) {
+            Path denylistPath = PathManager.getInstance().getHomePath().resolve("denylist.json");
+            Path allowlistPath = PathManager.getInstance().getHomePath().resolve("allowlist.json");
+            if (!Files.exists(denylistPath)) {
+                denylistPath = PathManager.getInstance().getHomePath().resolve("blacklist.json");
+            }
+
+            if (!Files.exists(allowlistPath)) {
+                allowlistPath = PathManager.getInstance().getHomePath().resolve("whitelist.json");
+            }
+            if (!Files.exists(denylistPath)) {
                try {
-                    Files.createFile(blacklistPath);
+                    Files.createFile(denylistPath);
                } catch (IOException e) {
-                    logger.error("IO Exception on blacklist generation", e);
+                    logger.error("IO Exception on denylist generation", e);
                }
            }
-            if (!Files.exists(whitelistPath)) {
+            if (!Files.exists(allowlistPath)) {
                try {
-                    Files.createFile(whitelistPath);
+                    Files.createFile(allowlistPath);
                } catch (IOException e) {
-                    logger.error("IO Exception on whitelist generation", e);
+                    logger.error("IO Exception on allowlist generation", e);
                }
            }
        }


As I understand this logic, it is not quite correct. I was not paying full attention last time to this area. Now it creates blacklist.json and whitelist.json files if no files are present, I think. It should never create new blacklist.json and whitelist.json files, so if those are present then the check for the corresponding new names can just be skipped.

All that this code should be doing is creating empty files if they do not exist, rather than trying to load them in. As such, it should not be creating files with the old names and should skip creating files with the new names if those old-named files are already present.

BenjaminAmos · 2024-09-29T18:30:45Z

engine/src/main/java/org/terasology/engine/network/internal/ServerConnectListManager.java

+        // although this seems redundant, compiler wouldn't accept assigning then checking
+        if (Files.exists(PathManager.getInstance().getHomePath().resolve("denylist.json"))) {
+            denylistPath = PathManager.getInstance().getHomePath().resolve("denylist.json");
+        } else {
+            denylistPath = PathManager.getInstance().getHomePath().resolve("blacklist.json");
+        }
+        // although this seems redundant, compiler wouldn't accept assigning then checking
+        if (Files.exists(PathManager.getInstance().getHomePath().resolve("allowlist.json"))) {
+            allowlistPath = PathManager.getInstance().getHomePath().resolve("allowlist.json");
+        } else {
+            allowlistPath = PathManager.getInstance().getHomePath().resolve("whitelist.json");
+        }


Suggested change

// although this seems redundant, compiler wouldn't accept assigning then checking

if (Files.exists(PathManager.getInstance().getHomePath().resolve("denylist.json"))) {

denylistPath = PathManager.getInstance().getHomePath().resolve("denylist.json");

} else {

denylistPath = PathManager.getInstance().getHomePath().resolve("blacklist.json");

}

// although this seems redundant, compiler wouldn't accept assigning then checking

if (Files.exists(PathManager.getInstance().getHomePath().resolve("allowlist.json"))) {

allowlistPath = PathManager.getInstance().getHomePath().resolve("allowlist.json");

} else {

allowlistPath = PathManager.getInstance().getHomePath().resolve("whitelist.json");

}

// Check for old naming first to retain compatiblity, otherwise use new naming by default.

Path potentialDenylistPath = PathManager.getInstance().getHomePath().resolve("blacklist.json");

Path potentialAllowlistPath = PathManager.getInstance().getHomePath().resolve("whitelist.json");

if (!Files.exists(potentialDenylistPath)) {

potentialDenylistPath = PathManager.getInstance().getHomePath().resolve("denylist.json");

}

if (!Files.exists(potentialAllowlistPath)) {

potentialAllowlistPath = PathManager.getInstance().getHomePath().resolve("allowlist.json");

}

denylistPath = potentialDenylistPath;

allowlistPath = potentialAllowlistPath;

denylistPath and allowlistPath are final variables, so they can only be assigned once in the constructor. Using a temporary variable here should work (but I wrote this quickly so please test that it compiles and works).

refactor: replaced all whitelist and blacklist occurences by allowlis…

b3415d9

…t and blocklist We want to demonstrate our openness and inclusiveness in our code Resolves MovingBlocks#5275

BenjaminAmos requested changes Sep 25, 2024

View reviewed changes

chore: fix checkstyle RightCurlyCheck errors

5f7294f

BenjaminAmos requested changes Sep 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replaced all whitelist and blacklist occurences by allowlis… #5277

refactor: replaced all whitelist and blacklist occurences by allowlis… #5277

byhlel commented Sep 23, 2024

Cervator commented Sep 23, 2024

byhlel commented Sep 23, 2024 •

edited

Loading

Cervator commented Sep 23, 2024

byhlel commented Sep 24, 2024

BenjaminAmos left a comment

BenjaminAmos Sep 25, 2024

byhlel Sep 26, 2024

BenjaminAmos Sep 25, 2024

BenjaminAmos Sep 25, 2024

byhlel Sep 26, 2024

BenjaminAmos Sep 25, 2024

byhlel Sep 26, 2024

byhlel commented Sep 26, 2024

BenjaminAmos Sep 29, 2024

BenjaminAmos Sep 29, 2024

	Terasology's engine uses allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:
	Terasology's engine uses an allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:

	Terasology's engine uses allowlisting approach to expose an API for modules using two primary methods and a rarely needed third one:
	Terasology's engine uses allowlisting to expose an API for modules using two primary methods and a rarely needed third one:

refactor: replaced all whitelist and blacklist occurences by allowlis… #5277

Are you sure you want to change the base?

refactor: replaced all whitelist and blacklist occurences by allowlis… #5277

Conversation

byhlel commented Sep 23, 2024

Contains

How to test

Outstanding before merging

Cervator commented Sep 23, 2024

byhlel commented Sep 23, 2024 • edited Loading

Cervator commented Sep 23, 2024

byhlel commented Sep 24, 2024

BenjaminAmos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byhlel commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byhlel commented Sep 23, 2024 •

edited

Loading