search

microsoft / do-client

Delivery Optimization client components
MIT License
24 stars 20 forks source link

Implement Generator parser argument #103

Closed mdavis777 closed 2 years ago

mdavis777 commented 2 years ago

Currently the generator parser argument causes the build to error out as a setter was never implemented.

ghost commented 2 years ago

CLA assistant check
All CLA requirements met.

shishirb-MSFT commented 2 years ago 
        self.generator = self.script_args.generator

Looks like the generator command line arg is already being consumed here. The issue maybe that self.generator is not returned from the getter methods.

Can you rework the fix to use self.generator instead of adding a new self._generator member?

Refers to: build/build.py:181 in fd0f527. [](commit_id = fd0f5274552034c28c6bc765e6bd6914fcf6bf12, deletion_comment = False)

mdavis777 commented 2 years ago 
        self.generator = self.script_args.generator

Looks like the generator command line arg is already being consumed here. The issue maybe that self.generator is not returned from the getter methods.

Can you rework the fix to use self.generator instead of adding a new self._generator member?

Refers to: build/build.py:181 in fd0f527. [](commit_id = fd0f527, deletion_comment = False)

No because self.generator is the function being called as the getter. If I change it to self.generator it is an infinite loop

shishirb-MSFT commented 2 years ago

Please see this commit: https://github.com/microsoft/do-client/commit/d714befc414e0967fcae65e855fb9920ea99b8d3

Base class provides the user specified cmdline argument and derived classes provide the platform-specific default when needed. This was the intended design but wasn't implemented correctly given that the Windows and Mac variations were added later.

In reply to: 985878609

shishirb-MSFT commented 2 years ago

@mdavis777 This has been fixed in main branch (see my other comment about the alternate fix). I will close this PR but please let me know if something is still missing for your use case.