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

Add option to upload languages configuration via API. #2170

Merged
merged 2 commits into from
Oct 1, 2023

Conversation

meisterT
Copy link
Member

Fixes #2142.

continue;
}
$lang->setAllowSubmit(true);
if (isset($language['name'])) {
Copy link
Member

Choose a reason for hiding this comment

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

We could use a PropertyAccessor loop here to get rid of most duplication. See also the problem import somewhere (I believe)

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this and it was not really less code / more understandable (probably also because of the nested compiler/runner fields). Feel free to submit to this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Then I’m fine as is

[$idField => $lang_id]
);
if (!$lang) {
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Should we not indicate this to the one uploading that the language is missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and no:

  • Logging is not really possible as we return the updated languages config, so that it can be compared in the script.
  • We could error out, but that's quite brutal.
  • We could try adding a new language, but of course stuff like executables are missing here.

What about this: I will add a TODO comment here for now; and when we decided on the best solution we can replace it?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that.

@meisterT meisterT merged commit 1279a02 into DOMjudge:main Oct 1, 2023
17 checks passed
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.

Allow uploading languages.json to the API
3 participants