Add command-line option for ConnectedService.json file by gregwinterstein · Pull Request #363 · OData/ODataConnectedService

gregwinterstein · 2023-06-13T18:56:32Z

Add command-line option for reading generation configuration from ConnectedService.json file
Command-line option: --connected-service-file <connected_service_file_name> or -c <connected_service_file_name>

Fixes #338
Fixes #327

habbes · 2023-08-09T13:42:49Z

+        /// <param name="configValue">Source value to use if not null or empty</param>
+        /// <param name="alternateValue">Alternate value to use if <paramref name="configValue"/> is null or empty</param>
+        /// <returns><paramref name="configValue"/> if not null or empty, otherwise <paramref name="alternateValue"/></returns>
+        private static string GetConfigValue(string configValue, string alternateValue)


Is this method necessary? I don't see how it's any better than the statement var value = string.IsNullOrEmpty(configValue) ? configValue : alternateValue. Also the name of the method is confusing given what it actually does. I thought it retrieving a value from a config store.

Method removed and replaced with ternary conditional.

habbes · 2023-08-09T13:44:35Z

+        {
+            CliConnectedServiceJsonFileData configFileData = null;
+
+            if (!string.IsNullOrWhiteSpace(fileName))


Maybe we should throw an exception when the fileName is null, instead of returning null.

The ConnectedServiceFile is not a required parameter and can be null/empty and that should not be an exception. If there is no ConnectedServiceFile parameter on the command-line, all of the configuration for the code generation will come from other command-line parameters.

nitpick: If the fileName is null, maybe we shouldn't call this method in the first place then?

Refactored GenerateCommand to avoid calling ReadConfigFile with a null fileName in f561b4c

habbes · 2023-08-18T15:46:54Z

+        {
+            CliConnectedServiceJsonFileData configFileData = null;
+
+            if (!string.IsNullOrWhiteSpace(fileName))


nitpick: If the fileName is null, maybe we shouldn't call this method in the first place then?

habbes · 2023-08-18T15:51:09Z

 {
    [DataContract]
-    public class UserSettings : INotifyPropertyChanged
+    public class UserSettings : CliUserSettings


nit: maybe: public class ConnectedServiceUserSettings : UserSettings

Renamed classes in c17c97d and ea9b9a8

gregwinterstein · 2023-08-22T20:47:05Z

Rebased to resolve conflicts in GenerateCommand.cs

gathogojr

@gregwinterstein Thank you for your pull request contribution. Sorry it took a bit of time to get round to doing a thorough review. Please take a look at my review comments at your earliest convenience.

KenitoInc · 2023-11-02T11:07:27Z

@gregwinterstein Kindly look into review comments by @gathogojr

gregwinterstein · 2023-11-02T14:13:36Z

@gregwinterstein Kindly look into review comments by @gathogojr

Hello @KenitoInc , I wlll respond as soon as I can. You all have caught me at an extremely busy time

gregwinterstein · 2023-11-14T23:24:46Z

@gregwinterstein Kindly look into review comments by @gathogojr

Hello @kentoinc & @gathogojr, I have reviewed your comments and updated the pull request as requested.

jvalkeejarvi · 2024-01-08T14:16:38Z

Hi! What is the situation of this PR? I'd really like to get the ConnectedService.json file support for CLI.

wachugamaina · 2024-03-07T19:46:18Z

Could we add some tests to this Pr ?

gregwinterstein · 2024-03-08T16:57:59Z

@wachugamaina

Could we add some tests to this Pr ?

This pull request adds a command-line option for reading generation configuration from ConnectedService.json file
Command-line option: --config-file <connected_service_file_name> or -c <connected_service_file_name>

The overwhelming majority of the changes are in src/Microsoft.OData.Cli/GenerateCommand.cs, inside private methods.

What would you like to see tested in this pull request?
How would you propose to test the changes in this pull request?

habbes · 2024-03-22T15:58:43Z

We do need to improve the test setup for the CLI, not just this PR (cc @ElizabethOkerio). See: #254

What would be a good starting point in testing the CLI is that the CLI options are correctly transformed into the expected ServiceConfig. And for the purpose of this PR, we would test whether the config in the JSON config files are correctly applied to the ServiceConfig. That would require some refactoring and setting a test project. The next step would be to consider some E2E tests, running the CLI and checking the results. Maybe that's beyond the scope of this PR? If it is, then something we should prioritize with a sense of urgency.

…quest

…ed in pull request comments

…unction with config-file option

…erly

…thogojr

…thogojr Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

gathogojr · 2024-03-26T15:32:21Z

@gregwinterstein Please take note of the following two review comments. Not sure you saw them since they were collapsed:

gregwinterstein · 2024-03-26T15:45:02Z

@gregwinterstein Please take note of the following two review comments. Not sure you saw them since they were collapsed:

* [Add command-line option for ConnectedService.json file #363 (comment)](https://github.com/OData/ODataConnectedService/pull/363#discussion_r1537580774)

* [Add command-line option for ConnectedService.json file #363 (comment)](https://github.com/OData/ODataConnectedService/pull/363#discussion_r1537610589)

@gathogojr Thank you for the reminder. I have replied to both of the comments you mentioned above. Please review.

gathogojr

Looks good. Thank you for your contribution @gregwinterstein

gregwinterstein force-pushed the fix/338-odata-cli-ConnectedService.json branch from cca12e3 to 17f5344 Compare June 15, 2023 18:52

ElizabethOkerio requested review from ElizabethOkerio, KenitoInc, gathogojr and habbes August 2, 2023 09:50

habbes reviewed Aug 9, 2023

View reviewed changes

gregwinterstein force-pushed the fix/338-odata-cli-ConnectedService.json branch from 17f5344 to a38b036 Compare August 9, 2023 23:17

habbes reviewed Aug 18, 2023

View reviewed changes

gregwinterstein force-pushed the fix/338-odata-cli-ConnectedService.json branch from f561b4c to 02dba47 Compare August 22, 2023 20:44

gathogojr force-pushed the fix/338-odata-cli-ConnectedService.json branch from 02dba47 to f510319 Compare September 25, 2023 06:32

gathogojr reviewed Sep 25, 2023

View reviewed changes

Comment thread src/ODataConnectedService.Shared/Models/ConnectedServiceUserSettings.cs Outdated

gregwinterstein force-pushed the fix/338-odata-cli-ConnectedService.json branch from f510319 to d894d1b Compare October 2, 2023 19:18

KenitoInc assigned habbes and gathogojr Oct 26, 2023

gathogojr reviewed Oct 27, 2023

View reviewed changes

wachugamaina reviewed Mar 8, 2024

View reviewed changes

Comment thread src/Microsoft.OData.Cli/GenerateOptions.cs

wachugamaina reviewed Mar 8, 2024

View reviewed changes

Comment thread src/Microsoft.OData.Cli/Models/ConnectedServiceFileData.cs

wachugamaina reviewed Mar 8, 2024

View reviewed changes

Comment thread src/Microsoft.OData.Cli/Models/ConnectedServiceFileData.cs

gregwinterstein force-pushed the fix/338-odata-cli-ConnectedService.json branch from 5b4b1fd to 92e6788 Compare March 11, 2024 15:31

gathogojr reviewed Mar 25, 2024

View reviewed changes

Comment thread src/Microsoft.OData.CodeGen/Models/BaseUserSettings.cs Outdated

gathogojr reviewed Mar 25, 2024

View reviewed changes

Comment thread src/Microsoft.OData.Cli/GenerateCommand.cs Outdated

gathogojr reviewed Mar 25, 2024

View reviewed changes

Comment thread src/Microsoft.OData.Cli/GenerateCommand.cs Outdated

gregwinterstein and others added 18 commits March 26, 2024 07:45

Correct logic to allow command-line options to override config file

08a964f

Correct string split options as suggested by pull request comments

bdb6af1

Improve variable name and simplify assignment per comments in pull re…

4f7c510

…quest

Simplify connected-service-file option name to config-file as suggest…

45bf8e5

…ed in pull request comments

Attempt to clarify optional/override behavior of metadata-uri in conj…

a28a44a

…unction with config-file option

Update summary comments to suggestions made in pull request comments

631e7e5

Set boolean defaults to null so that option override logic works prop…

08e4d37

…erly

Comment public properies as requested in pull request comments

380dae5

Post-rebase cleanup

8a04c25

Update src/Microsoft.OData.CodeGen/Models/BaseUserSettings.cs per @ga…

a165630

…thogojr Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs per @gathogojr

a6b14e0

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs per @gathogojr

e84942a

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs per @gathogojr

6f0757b

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs

1004c84

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs

efa0b1a

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs

818d62f

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Update src/Microsoft.OData.Cli/GenerateCommand.cs

e0c417f

Co-authored-by: John Gathogo <john.gathogo@gmail.com>

Fix variable name typo post-rebase

46da013

gregwinterstein force-pushed the fix/338-odata-cli-ConnectedService.json branch from 2ef77c7 to 46da013 Compare March 26, 2024 14:49

Correct invalid code reference to ConnectedServiceFileData

96ae9ee

Update exceptions per @gathogojr

2e68045

Correct exception thrown

563602b

gathogojr approved these changes Mar 27, 2024

View reviewed changes

ElizabethOkerio approved these changes Apr 3, 2024

View reviewed changes

habbes approved these changes Apr 3, 2024

View reviewed changes

gathogojr merged commit ff6294e into OData:master Apr 3, 2024

gregwinterstein deleted the fix/338-odata-cli-ConnectedService.json branch April 8, 2024 18:08

gathogojr mentioned this pull request May 6, 2024

Fix broken command line options #392

Merged

Conversation

gregwinterstein commented Jun 13, 2023 • edited by KenitoInc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregwinterstein Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregwinterstein commented Aug 22, 2023

Uh oh!

Uh oh!

gathogojr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KenitoInc commented Nov 2, 2023

Uh oh!

gregwinterstein commented Nov 2, 2023

Uh oh!

gregwinterstein commented Nov 14, 2023

Uh oh!

jvalkeejarvi commented Jan 8, 2024

Uh oh!

wachugamaina commented Mar 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregwinterstein commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

habbes commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gathogojr commented Mar 26, 2024

Uh oh!

gregwinterstein commented Mar 26, 2024

Uh oh!

gathogojr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

gregwinterstein commented Jun 13, 2023 •

edited by KenitoInc

Loading

gregwinterstein Aug 10, 2023 •

edited

Loading

gathogojr left a comment •

edited

Loading

gregwinterstein commented Mar 8, 2024 •

edited

Loading

habbes commented Mar 22, 2024 •

edited

Loading