Support toph #323

kfaRabi · 2019-02-19T07:21:28Z

Closes #302

Add support for Toph's problem downloading (from archive/collection), login and submission(in archive/collection).

No support for test case download and submission in contest mode yet as the contest pages are being loaded dynamically using JS.

…nto 'master' Resolve "Clone upstream locally and push to this repository" Closes online-judge-tools#1 See merge request toph/online-judge-tools!1

Resolve "Add support for Toph problem archive" Closes online-judge-tools#2 See merge request toph/online-judge-tools!2

kmyk

Thank you for your pull request! Your code is good!
But there are some points which can be better. Please fix these, and then I'll merge.

onlinejudge/service/toph.py

kmyk · 2019-02-19T10:00:36Z

onlinejudge/service/toph.py

+    def from_url(cls, s: str) -> Optional['TophProblem']:
+        result = urllib.parse.urlparse(s)
+        dirname, basename = posixpath.split(utils.normpath(result.path))
+        if result.scheme in ('', 'http', 'https') \


Suggested change

if result.scheme in ('', 'http', 'https') \

# example: https://toph.co/p/new-year-couple

if result.scheme in ('', 'http', 'https') \

kmyk · 2019-02-19T10:15:42Z

onlinejudge/service/toph.py

+        return None
+
+class TophProblem(onlinejudge.type.Problem):
+    def __init__(self, slug: str, kind: Optional[str] = None):


Suggested change

def __init__(self, slug: str, kind: Optional[str] = None):

def __init__(self, problem_id: str, contest_id: Optional[str] = None):

if contest_id is not None:

raise NotImplementedError

This argument kind is for the problems in the contest mode, right?
When we support the contest mode, it seems that this class need to have something like contest_id since the urls of problems in contets are like https://toph.co/c/tough-dash-feb-2019/arena#!/p/set-union .
So, we should use such names from now, and mark it as not-implemented explicitly.

.gitignore

kmyk · 2019-02-19T10:56:40Z

wow, I forgot to mention some things.

Please use the code formatters and the type checker. They are isort, yapf and mypy. We check them using the continuous integration, so the red cross mark and the message All checks have failed appear. How to run these commands is written here https://github.com/kmyk/online-judge-tools/blob/master/CONTRIBUTING.md#formatter. (I need to apologize that I didn't emphasize this and I didn't translate this to English when the guide which you asked me.)
(this is important, but you need not do this in this pull request) Please write tests. You should make tests/command_download_toph.py and edit tests/command_submit.py and tests/command_login.py. Tests are very important to write programs without bugs. Especially, this program are going to be broken automatically, since Toph may be updated. Therefore we also need to be monitoring bugs which will happen in the future. (by the way, I didn't say this too, sorry.)
(not so important) Please add the name of Toph to the message showed by --help.

for example, the one for the download subcommand is like below:

$ oj download --help
usage: oj download [-h] [-f FORMAT] [-d DIRECTORY] [--overwrite] [-n] [-a]
                   [-s] [--json]
                   url

positional arguments:
  url

optional arguments:
  -h, --help            show this help message and exit
  -f FORMAT, --format FORMAT
                        a format string to specify paths of cases (defaut: "sample-%i.%e" if not --system)
  -d DIRECTORY, --directory DIRECTORY
                        a directory name for test cases (default: test/)
  --overwrite
  -n, --dry-run         don't write to files
  -a, --system          download system testcases
  -s, --silent
  --json

supported services:
  Anarchy Golf
  Aizu Online Judge
  AtCoder
  Codeforces
  yukicoder
  CS Academy
  HackerRank
  PKU JudgeOnline
  Kattis

...

This is in onlinejudge/implementaion/main.py.

https://github.com/kmyk/online-judge-tools/blob/dfe74cc4095265f7e107db649ac123190ba6a08b/onlinejudge/implementation/main.py#L41-L51

kmyk · 2019-02-19T11:13:38Z

@kfaRabi by the way, the commits say you use two email addresses, and one of them is not associated with your GitHub account. Are you okay? If you want, you can fix this with something like git filter-branch --env-filter "........." origin/master and force pushing.

kfaRabi · 2019-02-19T11:32:04Z

@kmyk Thank you so much for taking the time to review the code and write suggestion + feedback. I am going to change everything as per your instructions soon. And, about the 2 emails, I actually work on GitLab. My initial commits were on GitLab, and once I was done, I committed here in GitHub. For this reason, you are probably seeing 2 emails.

Resolve "Add support for Toph problem archive" Closes online-judge-tools#2 See merge request toph/online-judge-tools!4

Resolve "Write tests for Toph" Closes online-judge-tools#5 See merge request toph/online-judge-tools!5

kfaRabi · 2019-02-25T09:25:04Z

@kmyk I have fixed a few things and added some tests. Could you kindly have a look at it now?

kmyk · 2019-02-25T20:55:50Z

tests/command_submit.py

+            subprocess.check_call([ojtools, 's', '-l', '58482c1804469e2585024324', '-y', '--no-open', url, 'a.py'], stdout=sys.stdout, stderr=sys.stderr)
+
+    @unittest.skipIf('CI' in os.environ, 'login is required')
+    def test_call_submit_add_them_up(self):


(not so important) It becomes better that this test case should submit C++ code, instead of Python code.

This tool guesses language ids (e.g. 584859c204469e2585024499 55b4af33421aa95e80000001 ...) from language names (e.g. C++14 C++11 C++ Python 3 PyPy 3 ...). The language names are different for each services and the guessing sometimes fails, so this case should also test this for C++.

kmyk · 2019-02-25T21:15:04Z

onlinejudge/service/toph.py

+        return None
+
+class TophProblem(onlinejudge.type.Problem):
+    def __init__(self, problem_id: str, kind: Optional[str] = None, contest_id: Optional[str] = None):


The variable kind seems to be not necessary for now (contest_id is None implies it is problem, and not None does contest).

I think that adding this variable is unnecessary generalization, and removing this is simple and better, but we should add this if there is another existing mode or a plan to add a new mode.
@kfaRabi Do you know another mode or a plan?

You are right. We do not need this as Toph does not plan to add any other mode soon.

There was: $ mypy oj onlinejudge setup.py tests onlinejudge/service/toph.py:151: error: Invalid index type "Optional[str]" for "Dict[str, str]"; expected type "str"

kmyk · 2019-02-25T21:25:01Z

@kfaRabi Thank you for fixing and adding tests.
CI failes yet and there are some additional comments, it is almost mergeable now.
Especially, the comments are just comments and I can merge this without fixing about it.

For CI, please cherry-pick adb59e2 and fd54382, then it will pass and I can/will merge this.

kmyk · 2019-02-25T21:44:35Z

@kfaRabi (and @hjr265) by the way, there are 3 questions:

Some commits have the name of @hjr265 as the commiter, but all of them are merge commits. Can I say "@kfaRabi implements this feature"? or both? (this is a question to update CHANGELOG.md)
Should I tell Toph to the Japanese community for competitive programming? It sounds good, but there is possibility that it increase the load of the site, and the top of standings of contests are filled with Japanese.
(about modes of Toph problems, one written as a review comment)

kmyk · 2019-02-26T08:03:07Z

onlinejudge/service/toph.py

+
+    def get_url(self) -> str:
+        # TODO: Check for contest_id to return the appropriate  URL when support for contest is added
+        return f'https://toph.co/p/{self.problem_id}'


Suggested change

return f'https://toph.co/p/{self.problem_id}'

return 'https://toph.co/p/{}'.format(self.problem_id)

f-string is useful and you should use it if possible, however, we need to support Python 3.5, and unfortunately Python 3.5 doesn't support f-strings.

hjr265 · 2019-02-26T08:11:59Z

@kmyk Regarding your questions:

This feature is being entirely implemented by @kfaRabi. My merge commits are there as we followed the workflow of our team like we do for all Toph related projects. Please feel free to credit him only for this contribution.
"top of standings of contests are filled with Japanese"... We would love to welcome the Japanese community to Toph. Please feel free to let them know. 🙂

Thank you for working with us closely as we implement this pull request.

kmyk · 2019-02-26T08:26:12Z

@hjr265 Thank you for answering. I'll let the community know Toph.

…setup.py tests

$ mypy oj onlinejudge setup.py tests onlinejudge/service/toph.py:151: error: Invalid index type "Optional[str]" for "Dict[str, str]"; expected type "str"

kmyk · 2019-02-26T09:30:53Z

CI succeeded 🎉 (once it failed, but it was for instability of tests)

kmyk · 2019-02-26T09:33:00Z

@kfaRabi I have merged your code. Thank you for your good work!

kfaRabi · 2019-02-26T09:37:33Z

@kmyk Thanks to you and @hjr265 for constant support and guidelines.

kmyk · 2019-02-26T10:03:27Z

I bumped version to 0.1.55. Now you can get the one which supports Toph with pip3 install -U online-judge-tools

Kazi Fozle Azim Rabi added 9 commits February 5, 2019 10:06

Add README.md

9ee1838

Merge branch '1-clone-upstream-locally-and-push-to-this-repository' i…

ba439a0

…nto 'master' Resolve "Clone upstream locally and push to this repository" Closes online-judge-tools#1 See merge request toph/online-judge-tools!1

Add .gitignore to avoid __pycache__ and test folders

b90926d

Add support for Toph problem archive

42d7e37

Implement passwordbased login for Toph

004fe62

Implement sumission for Toph

5985443

Update readme.md features section

5a49bd8

Merge branch '2-add-support-for-toph-problem-archive' into 'master'

bc0de2f

Resolve "Add support for Toph problem archive" Closes online-judge-tools#2 See merge request toph/online-judge-tools!2

Add support for Toph and fix merge conflicts

de350a8

kfaRabi changed the title ~~#302 Support toph~~ Support toph Feb 19, 2019

kmyk reviewed Feb 19, 2019

View reviewed changes

Kazi Fozle Azim Rabi and others added 2 commits February 19, 2019 14:04

Resolve "Add support for Toph problem archive"

bfc7e8b

Merge branch '2-add-support-for-toph-problem-archive' into 'master'

b96c44b

Resolve "Add support for Toph problem archive" Closes online-judge-tools#2 See merge request toph/online-judge-tools!4

kmyk added this to the v0.1.55 milestone Feb 20, 2019

kmyk assigned kfaRabi Feb 20, 2019

Kazi Rabi added 3 commits February 25, 2019 14:29

Write tests for Toph and do small fixes

8722bf1

Merge branch '5-write-tests-for-toph' into 'master'

048936b

Resolve "Write tests for Toph" Closes online-judge-tools#5 See merge request toph/online-judge-tools!5

Fix Merge Conflicts

9e504c9

kmyk reviewed Feb 25, 2019

View reviewed changes

kmyk added a commit that referenced this pull request Feb 25, 2019

#323: $ yapf --in-place --recursive oj onlinejudge setup.py tests

adb59e2

Add submission tests using CPP and remove unnecessary problem kind

ec58519

kmyk reviewed Feb 26, 2019

View reviewed changes

Use format instead of f

a99a9f2

kmyk added 2 commits February 26, 2019 15:09

online-judge-tools#323: $ yapf --in-place --recursive oj onlinejudge …

5fdfc4b

…setup.py tests

There was:

79de8d3

$ mypy oj onlinejudge setup.py tests onlinejudge/service/toph.py:151: error: Invalid index type "Optional[str]" for "Dict[str, str]"; expected type "str"

kmyk merged commit e5d4136 into online-judge-tools:master Feb 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support toph #323

Support toph #323

kfaRabi commented Feb 19, 2019

kmyk left a comment

kmyk Feb 19, 2019

kmyk Feb 19, 2019

kmyk commented Feb 19, 2019

kmyk commented Feb 19, 2019

kfaRabi commented Feb 19, 2019

kfaRabi commented Feb 25, 2019

kmyk Feb 25, 2019

kmyk Feb 25, 2019

kfaRabi Feb 26, 2019

kmyk commented Feb 25, 2019

kmyk commented Feb 25, 2019

kmyk Feb 26, 2019

hjr265 commented Feb 26, 2019 •

edited

Loading

kmyk commented Feb 26, 2019

kmyk commented Feb 26, 2019

kmyk commented Feb 26, 2019

kfaRabi commented Feb 26, 2019

kmyk commented Feb 26, 2019

	if result.scheme in ('', 'http', 'https') \
	# example: https://toph.co/p/new-year-couple
	if result.scheme in ('', 'http', 'https') \

-    def __init__(self, slug: str, kind: Optional[str] = None):
+    def __init__(self, problem_id: str, contest_id: Optional[str] = None):
+        if contest_id is not None:
+            raise NotImplementedError

	return f'https://toph.co/p/{self.problem_id}'
	return 'https://toph.co/p/{}'.format(self.problem_id)

Support toph #323

Support toph #323

Conversation

kfaRabi commented Feb 19, 2019

kmyk left a comment

Choose a reason for hiding this comment

kmyk Feb 19, 2019

Choose a reason for hiding this comment

kmyk Feb 19, 2019

Choose a reason for hiding this comment

kmyk commented Feb 19, 2019

kmyk commented Feb 19, 2019

kfaRabi commented Feb 19, 2019

kfaRabi commented Feb 25, 2019

kmyk Feb 25, 2019

Choose a reason for hiding this comment

kmyk Feb 25, 2019

Choose a reason for hiding this comment

kfaRabi Feb 26, 2019

Choose a reason for hiding this comment

kmyk commented Feb 25, 2019

kmyk commented Feb 25, 2019

kmyk Feb 26, 2019

Choose a reason for hiding this comment

hjr265 commented Feb 26, 2019 • edited Loading

kmyk commented Feb 26, 2019

kmyk commented Feb 26, 2019

kmyk commented Feb 26, 2019

kfaRabi commented Feb 26, 2019

kmyk commented Feb 26, 2019

hjr265 commented Feb 26, 2019 •

edited

Loading