search

dj3500 / hightail

MIT License
271 stars 144 forks source link

putting the problem's name when you parse the problem statement thro… #97

Closed jfsanin closed 7 years ago

jfsanin commented 7 years ago

…ugh the url, as a default name for searching the executable.

Added configuration options for choosing the problem name (only tested in codeforces) Added some unit test

dj3500 commented 7 years ago

Thanks! You may need to wait a little before I get to looking at this; sorry about that.

jfsanin commented 7 years ago

No worries!.

dj3500 commented 7 years ago

Ok, I finally got around to look at this - sorry it took so long!

A few comments/requests:

  1. Please rebase on the current state of master.
  2. There are some empty event handlers (firstLetterRadioButtonActionPerformed and another one), let's remove them.
  3. I guess it would be better if there was a single checkbox instead of two radio buttons.
  4. Awesome that you created a test! Now we have > 0% test coverage :P
  5. Let's not change the interface of parse() by adding the boolean, but instead let's just look up the value from the Config class where it is needed (i.e., in the CodeForces task parser).

Thanks a lot for the contribution!

jfsanin commented 7 years ago

Do not worry for taking your time to look this, there is no problem.

I also have few comments: 1 there is no problem. 2 I agree 3 I agree 4 haha, the project deserved some test :) 5 Well, when I changed the interface I thought that in the future every site can offer the option, not just CodeForces, actually I think the Tool get complete problem's name for almost every site, so I don't know if don't change that interface and look up the value in every class is a good idea.

Thank you for take your time and check my contribution.

dj3500 commented 7 years ago

For 1, automatic rebase is possible but results in non-compiling code because the new OpenKattis parser has still the old interface. But once 5 is implemented, this should not matter (all the more reason to do it :) ).

For 5, there is actually another pull request right now which also wants to add some extra argument to these calls... :) I see no reason to not have this in the config. It is, after all, a configuration parameter. Every parser which wants to use it can look it up from the Config object.

jfsanin commented 7 years ago

I have done the changes, please check them out.

-I have added a formatter because with some special characters in the problem name, my O.S seem to have some problems.

Any suggestion, or any question are welcome.

dj3500 commented 7 years ago

Thanks! I have made a commit with some polishing and removing unnecessary changes (https://github.com/dj3500/hightail/commit/c9e2ba2d181e65df79283529a363781c63f45dc8), and merged. (This is the diff of the entire pull request with my changes: https://github.com/dj3500/hightail/commit/296dd834a2cb6a071ad0f5dcb92dd817dae844a4.)

I still have a question, however. Let's say you enable this option; then you get generated paths like "c:\whatever\A. Bachgold Problem.exe". Do you actually name your files like this?

jfsanin commented 7 years ago

Hi,

Maybe it is a little weird, but yes, I really like putting the whole name and not a generic name like "A" or "1" or other things.

do you see any disadvantages in this?

dj3500 commented 7 years ago

Not really. In a contest it could be time-consuming, but I guess you do it for practice/training. Just wondering for how many people this feature could be useful :)

Another nice thing might be to have the full name in the tab label, but then only A.exe as the filename. But I guess that would be just aesthetics.

Thanks again for the contribution!