Read in `MINECRAFT_ROOT` as an environment variable

[OpenBagTwo]

GIVEN Alex has EnderChest installed on her system, say at the location /mnt/drive/minecraft/EnderChest

WHEN she opens a terminal and runs the command

$ enderchest open

forgetting to either specify --root /mnt/drive/minecraft/ or first cd to that directory

THEN instead the operation should succeed instead of her getting the error message:

Could not load EnderChest from /home/alex:
  No valid EnderChest installation exists within /home/alex

BECAUSE she had already added the line export MINECRAFT_ROOT=/mnt/drive/minecraft/ to her .bashrc

SO that she can be lazy in her running off EnderChest commands

Notes

[JayPankajPatel]

Hey could you assign the issue to me.

[OpenBagTwo]

Have at it!

[JayPankajPatel]

I'm a bit lost on where exactly to implement this I was thinking here

https://github.com/OpenBagTwo/EnderChest/blob/3b72d2e117cd71bec03e38aee143ef8c8fe7af1d/enderchest/filesystem.py#L7-L12 or should I leave this file alone I want to read the env vars with os, see if MINECRAFT_ROOT exists, and then replace every Path var with that root. Let me know if that makes sense or is breaking previous implementations.

[JayPankajPatel]

Just a heads up this is my first non-trival attempt at open source contributions

[OpenBagTwo]

Good thought, but probably the best place to do this is in cli.py, as the intent is to infer the Minecraft root if and only if the user does not provide it.

A good place to start is to look at how the CLI currently handles it when the user does not provide the --root argument explicitly.

[JayPankajPatel]

I propose to embed the logic directly in parse_args and pass the MINECRAFT_ROOT env var as the first value in line 544 here and change it to Path(MINECRAFT_ROOT or root_arg or root_flag or os.getcwd()), after adding the proper logic to read and check if the env var exists https://github.com/OpenBagTwo/EnderChest/blob/3b72d2e117cd71bec03e38aee143ef8c8fe7af1d/enderchest/cli.py#L542-L546

[OpenBagTwo]

That sounds like the cleanest implementation. But remember that MINECRAFT_ROOT should only be used if neither root_flag or root_arg are used.

Before doing your implementation have you considered writing unit tests? I'm a huge believer in test-driven design.

[JayPankajPatel]

Ah I see so we would change Path(MINECRAFT_ROOT or root_arg or root_flag or os.getcwd()) to this Path(root_arg or root_flag or MINECRAFT_ROOT or os.getcwd()) I have not written unit tests nor know how to in python, I will do more research, look at existing tests, and write my unit tests before changing anything. But I don't actually have Minecraft installed on this computer, what should I do?

[OpenBagTwo]

You don't need Minecraft installed (you wanna know a secret? Very little in this project is actually Minecraft-specific--it could just as easily apply to any game that players tend to run with multiple mods or configurations).

Anyway, just follow the contributor's guide to get set up.

No worries if you haven't written unit tests before. Have a look at test_cli.py, explicitly the tests around the root argument in ActionTestSuite and see if you can come up with a few tests.

The coverage we'll need is testing that:

• A MINECRAFT_ROOT environment variable will be used instead of the current working directory if such a variable is set
• If MINECRAFT_ROOT is not set, the current working directory will be used instead
• a user-supplied root argument always takes priority

In terms of how you actually write tests around environment variables, have a look at this guide and let me know if you have questions.

As an alternative, I can write the tests for you, and you'll know your implementation is good once those tests all pass.

[JayPankajPatel]

Hey thanks for the detailed response, wow that makes sense, I was wondering why some of the modules were so engineered. I will write unit tests that will test

• if no explicit --root flag is given, and the root is not in the working directory and MINECRAFT_ROOT exists
• if no explicit --root flag is given, and the root is in the working directory and MINECRAFT_ROOT exists
• if explicit --root flag is given and it overwrites the existing parameters of working directory and MINECRAFT_ROOT
• if nothing is given, it should print the existing error message and additionally a no env var MINECRAFT_ROOT is set and raise some sort of exception, (maybe an OSException)?

Do these tests have enough coverage? and the expected behavior you want?

[OpenBagTwo]

That's not quite right. Does this flowchart help?

[JayPankajPatel]

I simply wrote the following, what do you think? Also how did you setup this projects, i love it, I'm starting a large python project and want to the tools that were used here, could you point me to some resources?

def test_root_can_also_be_provided_by_systemenv(self, monkeypatch):
        monkeypatch.setenv("MINECRAFT_ROOT", "/mnt/drive/minecraft/")
        _, root, _, _ = cli.parse_args(["enderchest", *self.action.split(), *self.required_args])
        assert root == Path("/mnt/drive/minecraft/")

[OpenBagTwo]

Yes. Exactly.

[JayPankajPatel]

Cool, I simply did the following according to the flow chart, I match the order. I believe the short-circuit optimization the interpreter makes will let me do simply this

            MINECRAFT_ROOT = os.getenv("MINECRAFT_ROOT")

            return (
                actions[aliases[command]],
                Path(root_arg or root_flag or MINECRAFT_ROOT or os.getcwd()),
                log_level,
                action_kwargs,
            )

MINECRAFT_ROOT will be set to None rather than raising an error if it doesn't exist

[OpenBagTwo]

Eager to review your PR!

[OpenBagTwo]

Hey @JayPankajPatel any progress on this PR? Last we talked you'd pretty much gotten it figured out, and I would love to include this in the next release

[JayPankajPatel]

Hey @OpenBagTwo I forgot to close this, I will reread the contributions guide, update documentation, and create a merge request today.