Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly set default repository name. #1122

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Correctly set default repository name. #1122

merged 1 commit into from
Nov 1, 2024

Conversation

gouttegd
Copy link
Contributor

@gouttegd gouttegd commented Nov 1, 2024

If the ODK seeding configuration file does not define the name of the repository (repo) and if no name is explicitly given on the command line, we want to set a default repository name ("noname") to prevent a crash when generating the files.

Unfortunately, the way this was done (#1097) makes any repo value defined in the configuration ignored, because we:

  • first set repo to "noname" if we didn't get anything from the command line;
  • then call load_config with whatever value we have for repo (hether it comes from the command line or from our default), and load_config takes whatever value we give it and overrides any value it may have found in the config file.

So here, we define a default value for project.repo at the moment that field is declared (as for all other default values), and do not set a default value in the main function.

This way:

  • if no repo argument is given in the command line, we get the value from the configuration file, or the default value if there is repo key in the config file;
  • if a repo argument is given in the command line, it overrides both any value that may be set in the configuration file, and the default value.

If the ODK seeding configuration file does not define the name of the
repository (`repo`) and if no name is explicitly given on the command
line, we want to set a default repository name ("noname") to prevent a
crash when generating the files.

Unfortunately, the way this was done (#1097) makes any `repo` value
defined in the configuration _ignored_, because we:

* first set `repo` to "noname" if we didn't get anything from the
  command line;
* then call `load_config` with whatever value we have for `repo` (hether
  it comes from the command line or from our default), and `load_config`
  takes whatever value we give it and **overrides** any value it may
  have found in the config file.

So here, we define a default value for `project.repo` at the moment that
field is declared (as for all other default values), and do **not** set
a default value in the main function.

This way:

* if no `repo` argument is given in the command line, we get the value
  from the configuration file, or the default value if there is `repo`
  key in the config file;
* if a `repo` argument is given in the command line, it overrides both
  any value that may be set in the configuration file, and the default
  value.
@gouttegd gouttegd self-assigned this Nov 1, 2024
@gouttegd gouttegd requested a review from matentzn November 1, 2024 19:32
Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming it's tested, looks good!

@gouttegd
Copy link
Contributor Author

gouttegd commented Nov 1, 2024

I specifically tested running the seeding script with various combinations (config file with or without a repo key, and call with or without a repo name on the command line), and it all worked as expected.

(Then again, the changes in #1097 were tested, too… and yet I missed the fact that they introduced the bug I am fixing here. ^^' )

@gouttegd gouttegd merged commit 57b612e into master Nov 1, 2024
1 check passed
@gouttegd gouttegd deleted the fix-default-repo branch November 1, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants