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

add api endpoint for commitment merging #661

Merged
merged 7 commits into from
Feb 27, 2025
Merged

add api endpoint for commitment merging #661

merged 7 commits into from
Feb 27, 2025

Conversation

Varsius
Copy link
Contributor

@Varsius Varsius commented Feb 11, 2025

Checklist:

  • I updated the documentation to describe the semantical or interface changes I introduced.

@Varsius Varsius requested a review from majewsky February 19, 2025 10:10
@coveralls
Copy link

coveralls commented Feb 19, 2025

Coverage Status

coverage: 79.465% (+0.04%) from 79.422%
when pulling 614d361 on commitment-merging
into 7cc255c on master.

Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

I will do a more detailed review later today or tomorrow, but for the time being, here are comments on the DB migration and model:

@Varsius Varsius force-pushed the commitment-merging branch 2 times, most recently from 06a7a95 to a6f38ab Compare February 20, 2025 12:33
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

FYI, I read a bit more (didn't get to finish again....). To make the feedback cycle shorter, I added some commits for minor stuff. Please review my commits, esp. the one touching the migrations.

Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

We are nearing completion here. I pushed another commit with just whitespace changes (esp. in tr.DBChanges().AssertEqualf(); that fellow normalizes whitespace on its input, so you can make liberal use of indentation to make the test code look neat).

Finally, since #667 was merged, you will need to do a rebase. There will be a few merge conflicts.

@Varsius Varsius requested a review from majewsky February 26, 2025 14:58
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

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

When you merge this, please take a look at the qa-de-1 database to see how the DB migration goes.

@Varsius Varsius merged commit c64b321 into master Feb 27, 2025
7 checks passed
@Varsius Varsius deleted the commitment-merging branch February 27, 2025 12:15
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.

3 participants