BAPCtools icon indicating copy to clipboard operation
BAPCtools copied to clipboard

[tools] Separate changing to contest directory and getting problems list

Open mpsijm opened this issue 2 years ago • 4 comments

Copying part of the discussion from https://github.com/RagnarGrootKoerkamp/BAPCtools/pull/163#discussion_r748593896:

@RagnarGrootKoerkamp:

Really it would be cleaner to parse these after cding to the contest dir, but that probably introduces other problems

@mpsijm:

I've tried separating the cding to the contest dir before parsing the personal config file in f5ac03601745ae8935d89209356b8b5ba8862cd2. [...] Splitting off a change_directory() function from the get_problems() function already makes things slightly cleaner, but:

  • Moving change_directory() outside run_parsed_args means we'll have to duplicate this between main() and test(args)
  • This will also require changes in the behaviour of new_{contest,problem}, as the --contest and --problem flags are not valid for these commands

mpsijm avatar Nov 12 '21 22:11 mpsijm

  • Moving change_directory() outside run_parsed_args means we'll have to duplicate this between main() and test(args)

That's fine - duplicating some code for tests is OK with me.

  • This will also require changes in the behavior of new_{contest,problem}, as the --contest and --problem flags are not valid for these commands

Right. Do you want to spend some more time on this for this PR? (I assume that's why it's draft for now.)

I'm not exactly sure myself yet how we should handle this. One possibility:

  1. parse args without reading any config files (only to find the subcommand)
  2. if new_{contest,problem}, handle that and exit
  3. if other subcommand, cd to contest directory
  4. parse args again, now with reading the required context

RagnarGrootKoerkamp avatar Nov 13 '21 20:11 RagnarGrootKoerkamp

duplicating some code for tests is OK with me.

All right :slightly_smiling_face:

(I assume that's why it's draft for now.)

Correct :yum:

I'm not exactly sure myself yet how we should handle this. One possibility: [...]

Sounds good, but I don't like the fact that the arguments need to be parsed twice. I was thinking of something like the following (but I'll need to try this to see how it works out):

  1. In main():
    1. Read command line args (without passing the personal config as default values)
    2. Set initial_cwd = os.getcwd()
    3. cd to to contest directory (this reads --contest and --problem only from the command line, but having them in a personal config file wouldn't make sense anyway :stuck_out_tongue:)
    4. Read personal config
    5. Call run_parsed_arguments with combined args object
  2. In run_parsed_arguments(args):
    1. if action in ['new_contest', 'new_problem']: check if --contest and/or --problem are set, and throw an error if so (only new_problem --contest would be valid). Then, os.chdir(initial_pwd) and continue with the action
    2. else: continue processing the args as usual

Maybe 2.i should be checked before 1.iv, but that requires a bit more rewriting. I'll experiment with it for a bit :slightly_smiling_face:

mpsijm avatar Nov 14 '21 11:11 mpsijm

@mpsijm what's the status here? I kinda forgot what this was about

RagnarGrootKoerkamp avatar Dec 10 '22 12:12 RagnarGrootKoerkamp

If I remember correctly, this is mostly to make the parsing of the personal config file more consistent. It's mostly a code cleanup that doesn't really have high priority, but I would still like to finish it if I do have the time :stuck_out_tongue:

mpsijm avatar Dec 11 '22 15:12 mpsijm