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

Create group #5

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Create group #5

merged 3 commits into from
Aug 5, 2024

Conversation

otulach
Copy link
Contributor

@otulach otulach commented Jul 26, 2024

No description provided.

@otulach otulach force-pushed the create-group branch 3 times, most recently from 3fb0874 to 789169b Compare August 1, 2024 08:57
@vhotspur
Copy link
Member

vhotspur commented Aug 1, 2024

Few ideas on how to extend this one:

  • allow specification of --path and --name to allow naming the group directly
    • both as templated fields expanded from CSV columns
    • if --name is missing, it is taken from --path (--path is required)
    • this would be really nice to have :-)
  • automatically create parent group(s) if they do not exist
    • running as --from parent --path 'a/{name}' would first create parent/a group and inside that one the group {name} as specified in the CSV column name
    • this might be nice but is not required :-)

@otulach
Copy link
Contributor Author

otulach commented Aug 3, 2024

So, this recent commit should take care of it.

There is now an option to specify --name and diffefentiate the actual group name
from --path, which, in contrast, is required.

Copy link
Member

@vhotspur vhotspur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that having at least one test would be really nice but apart from the tiny suggestions this is in a very good shape. Thanks!

src/teachers_gitlab/main.py Outdated Show resolved Hide resolved
src/teachers_gitlab/main.py Outdated Show resolved Hide resolved
src/teachers_gitlab/main.py Outdated Show resolved Hide resolved
src/teachers_gitlab/main.py Outdated Show resolved Hide resolved
src/teachers_gitlab/utils.py Outdated Show resolved Hide resolved
@otulach
Copy link
Contributor Author

otulach commented Aug 4, 2024

All of the comments were helpful and made sense to me.
They are now implemented in the last commit. :)

Copy link
Member

@vhotspur vhotspur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks a lot! Merging in the current state, tests can come in later PRs.

@vhotspur vhotspur merged commit add0a0d into d-iii-s:master Aug 5, 2024
1 check passed
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