-
Notifications
You must be signed in to change notification settings - Fork 432
Add Node.js to list of languages on README and demo languages #2297
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
base: master
Are you sure you want to change the base?
Conversation
README.md
Outdated
| * GAS x86/x64/ARM | ||
| * Haskell | ||
| * INTERCAL | ||
| * JavaScript (Node.js, V8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose for consistency with the C++ variants this should say "(Node.js and V8)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to say Node.js and V8!
9e9fcc7 to
d93b90e
Compare
judge/fixtures/language_all.json
Outdated
| "fields": { | ||
| "key": "NODEJS", | ||
| "name": "JavaScript (Node.js)", | ||
| "short_name": "Node.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short name is used on the submission listing. For consistency, it should be all uppercase. If you leave it as null, then key will be used instead (which seems fine in this case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call: I changed this to null, and the key should work well here
| "pk": 77, | ||
| "fields": { | ||
| "key": "NODEJS", | ||
| "name": "JavaScript (Node.js)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming sounds good to me -- could we also rename V8 to "JavaScript (V8)" for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to JavaScript V8!
|
Thanks for the review @Xyene and sorry about the extreme lateness of this follow-up. I just made the requested changes, so this should probably be good to go. I'm not sure who's the best to merge this in, but @kiritofeng and @quantum5, might either of you be open to taking a quick look? (I see you've made some of the most recent merges) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2297 +/- ##
==========================================
+ Coverage 46.78% 47.72% +0.94%
==========================================
Files 251 260 +9
Lines 13309 13745 +436
==========================================
+ Hits 6226 6560 +334
- Misses 7083 7185 +102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Motivation
Related to DMOJ/judge-server#1143:
After enabling support in judge-server for a Node.js-based executor, we need add the language to the demo instance and the documentation
Changes
language_all.json)Testing
Ran
python3 manage.py loaddata language_alland verified the languages were loaded properly in the Admin interface