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

Gtad generating. BaseJob. #15

Merged
merged 21 commits into from
Jul 16, 2021
Merged

Gtad generating. BaseJob. #15

merged 21 commits into from
Jul 16, 2021

Conversation

Aksem
Copy link
Collaborator

@Aksem Aksem commented Jul 7, 2021

No description provided.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Thanks, the direction looks quite sensible; some feedback on the current state below.

.vscode/cmake-variants.yaml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
PyQuotient/event-schemas/schema/m.room.member.xml Outdated Show resolved Hide resolved
tests/jobs/test_basejob.py Show resolved Hide resolved
tests/jobs/test_basejob.py Show resolved Hide resolved
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Looks much better! A couple of suggestions from me.

gtad/typesystem.xml.mustache Outdated Show resolved Hide resolved
tools/patch_gtag_config.py Outdated Show resolved Hide resolved
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Ok, I see a lot of progress - great job - but we have to cut this PR somewhere because it starts spinning out of control. It was originally aimed at getting the jobs working, and I totally get that Connection might be useful for that; but Room should really be spared for another PR. At this point it's worth spending a bit more effort at making the CI working.

@Aksem Aksem marked this pull request as ready for review July 16, 2021 13:46
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

Awesome, CI is working - if you wish you can move the Room bindings out but since it's not the final PR anyway, it even is ok to keep it.

This was referenced Jul 16, 2021
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