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

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpsijm
Copy link
Collaborator

@mpsijm mpsijm commented Nov 12, 2021

Copying part of the discussion from #163 (comment):

@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 f5ac036. [...] 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

@RagnarGrootKoerkamp
Copy link
Owner

RagnarGrootKoerkamp commented Nov 13, 2021

  • 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

@mpsijm
Copy link
Collaborator Author

mpsijm commented Nov 14, 2021

duplicating some code for tests is OK with me.

All right 🙂

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

Correct 😋

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 😛)
    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 🙂

@RagnarGrootKoerkamp
Copy link
Owner

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

@mpsijm
Copy link
Collaborator Author

mpsijm commented Dec 11, 2022

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 😛

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.

2 participants