[4.0] Adding directory safety to the binder.

[Lyrcaxis]

Purpose of this PR

Binder can now be started from inside the bin folder.

Testing status

• Tested running from inside the bin folder.

Comments

I believe being able to obtain a copy of the source code with as less headache as possible will attract more contributors.
This should provide one headache less for people whose binder runs from inside the bin folder.

[Nihlus]

I'm not sure this is the right approach. The binder should make as few assumptions as possible, and the intent is for it to run as a configured step in the build process. If that step isn't working, we should focus on fixing that.

[Nihlus]

Thanks for taking the time and showing interest, though!

[Lyrcaxis]

Could still serve as a temporary solution, especially while a build isn't available, as people will have that problem often.. but it's up to you.

[Nihlus]

Maybe we could create a set of setup scripts that compiles & executes the binder with the correct arguments in the correct directory instead?

[varon]

@Lyrcaxis - Thanks. We hugely appreciate this work.

@Nihlus if it makes it easier for contributors we should merge it. The adjustments are simple and the fix is good. If someone actually wants to use the binder in other projects for other stuff THEN it would make sense to change it, but as things stand, this is a significant improvement to usability.

[Perksey]

Yeah. I’m all for merging, but the directory structure in the binder needs refactoring in general, but not in 4.0. So let’s say merge and create an issue so we remember to revisit this?

[varon]

@Lyrcaxis - Sat down with @Nihlus for a chat about the ins and out of this.

We're super super grateful for the contribution. It's nice to see someone digging deeper into the internal bits for stuff like this. We're ready to merge, but if we're going this far, we'd like to go just a little further and get the binder handle path choice a little bit more elegantly.

• Move the path-finding logic into a separate static class. PathResolver perhaps?
• If the path arguments are not present, notify the user and call the new functionality in PathResolver.
• Fail fast with a clear error message if no valid path can be found in the resolver.

The idea you've had is really good. We'd love to see it merged - just need to get the last bit sorted! It shouldn't take very long to make these adjustments- reckon you could give it a shot?

[Lyrcaxis]

I'm on it. Just a thing, I dont know how to check if the path arguments are there.

[Lyrcaxis]

Hang on. Just got an idea.

[Lyrcaxis]

I think this should do it.
Could also add specific update instead of batch update in SetDefaults() and UpdateArguments() to leave space for any additional magic (??)

[Nihlus]

@Lyrcaxis Nice work! Had a read through - I see you've added some manual verification and checking of the arguments, but there's no need for that. If you assign a default value to the property in the class, it'll use that if no argument was passed at the command line.

Some thoughts about the algorithm, too - if a path has been passed on the command line, we probably shouldn't do anything with it - it's up to the user to pass correct paths if they're manually overriding things.

[Lyrcaxis]

Nice work! Had a read through - I see you've added some manual verification and checking of the arguments, but there's no need for that. If you assign a default value to the property in the class, it'll use that if no argument was passed at the command line.

@Nihlus Thanks! Yeah I just got overexcited I guess. It just felt weird to just check if (args.Length == 0) and every other solution was prone to false-positives.

Some thoughts about the algorithm, too - if a path has been passed on the command line, we probably shouldn't do anything with it - it's up to the user to pass correct paths if they're manually overriding things.

Sure, though even in those cases, it wouldn't hurt to try and correct the user, since the algorithm directly scans for existing directories/files. I can rewrite that real quick if you think it shouldn't happen.

[Nihlus]

@Lyrcaxis Yeah, I think we shouldn't do anything with user-supplied arguments - it's rather unexpected behaviour.

[Lyrcaxis]

It should be good now.

[Lyrcaxis]

Functionality should be exactly the same though.. I think this update was pretty useless..
If the arguments aren't validated but are invalid, the program would exit without giving the user a clue of why.
Well atleast the console output is a bit thinner now :P

[Nihlus]

You're still manually checking for missing arguments - there's no need to do that. If an argument is missing from the command line, the default property value will be used instead. I'd think the easiest way would be to wrap up your scanning functionality in a method that returns the found path, and just assign the result of that function as a default value.

[Lyrcaxis]

I modified the way the default arguments are assigned. They were assigned via a default property value and now they're assigned via the validation function. Functionality should remain the same.

[Nihlus]

The thing is, you've got a bunch of extra code now that would need to be updated if we ever added or changed an option for some reason (the helper enum, the GetPathByID, the SetDefaultPath functions, etc). It's fairly rickety as it is. If you use the intended way of assigning a default value to an option, you won't need these extra bits.

[Perksey]

I don’t think this is the right way to go. If the problem is with the way the binder is handling the path provided, then sure this might be needed. But the problem is clearly with how we invoke the binder, so we should focus on that. As long as the generator actually works in our build steps, that’s all that matters as this is an internal tool - we don’t need to focus on validation.

[Lyrcaxis]

Alright. Should be how you wanted it now :P

[Perksey]

I don't think we should be focusing on validation as much and, as such, I don't think we need this PR. Don't get me wrong, this would be amazing if we were looking to make a user-friendly and thank you for your time on it; however we don't need to go too in-depth with validation as this is a tool that is executed only by our build steps.

[varon]

@Lyrcaxis - I'll try to branch off from what you've done and get this in.