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.

Sign up for GitHub

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

Jump to bottom

🐛 Correct properties for attachment model #31

Merged
merged 4 commits into from
Nov 3, 2021
Merged

🐛 Correct properties for attachment model #31

merged 4 commits into from
Nov 3, 2021

Conversation

s0
Copy link
Contributor

@s0 s0 commented Nov 3, 2021

Stricter types can be used for objectType and type, which was the case
for the old attachment model, so we should continue that here.

@s0
🐛 Correct properties for attachment model
ecef10f 
Stricter types can be used for objectType and type, which was the case
for the old attachment model, so we should continue that here.
@s0 s0 added the ready for review All comments have been addressed, and the Pull Request is ready for review label Nov 3, 2021
@s0 s0 assigned czmj and Pl217 Nov 3, 2021
@s0 s0 requested a review from a team as a code owner November 3, 2021 11:34
Pl217
Pl217 requested changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@Pl217 Pl217 left a comment

Choose a reason for hiding this comment

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

Since, we're enriching attachment definition, I would take the opportunity to remove unused variable COST_ATTACHMENT_VALUE that is duplicated from attachmentVersion.ts, where it is actually used. Both were added in f59ddc1

Can I take this PR to sneak in a commit that removes this code comment since it doesn't belong after the changes we did today in 948d97d?

@Pl217 Pl217 assigned s0 and unassigned Pl217 Nov 3, 2021
@Pl217 Pl217 added needs minor changes There are review or issue comments to address and removed ready for review All comments have been addressed, and the Pull Request is ready for review labels Nov 3, 2021
@Pl217
Copy link
Contributor

Pl217 commented Nov 3, 2021

Also, what are your plans for HPC-8205 branch that removes COST_ATTACHMENT_VALUE (which I asked above, before realizing this 😄) and also makes changes to attachmentPrototype and attachmentVersion?

@s0
Copy link
Contributor Author

s0 commented Nov 3, 2021

Since, we're enriching attachment definition, I would take the opportunity to remove unused variable COST_ATTACHMENT_VALUE that is duplicated from attachmentVersion.ts, where it is actually used. Both were added in f59ddc1

Can I take this PR to sneak in a commit that removes this code comment since it doesn't belong after the changes we did today in 948d97d?

sounds good to me

Also, what are your plans for HPC-8205 branch that removes COST_ATTACHMENT_VALUE (which I asked above, before realizing this 😄) and also makes changes to attachmentPrototype and attachmentVersion?

That's still a WIP that i need to evolve a bit more, but i'll just rebase that ontop of whatever other changes we have in hpc-api-core, and will only finalize it when the related code in hpc_service is final and ready too, which I hope to have by end of day.

@s0
Copy link
Contributor Author

s0 commented Nov 3, 2021

@Pl217 done

@s0 s0 assigned Pl217 and unassigned s0 Nov 3, 2021
@s0 s0 added ready for review All comments have been addressed, and the Pull Request is ready for review and removed needs minor changes There are review or issue comments to address labels Nov 3, 2021
s0 and others added 2 commits November 3, 2021 15:37
@s0 @Pl217
📝 Remove irrelevant comment
a789af6 
This comment was meant to "justify" why 'enum' was used
for kind property when projectVersion got defined in
f3fdb87

It was corrected later in 948d97d
to use 'checked' kind with `t.keyof()` io-ts type
@Pl217
🔖 Bump version to v0.2.1
5963b71
@Pl217 Pl217 force-pushed the attachment-improvements branch from 4514cb8 to 5963b71 Compare November 3, 2021 14:40
Pl217
Pl217 approved these changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@Pl217 Pl217 left a comment

Choose a reason for hiding this comment

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

I have also bumped version here, because I will publish a new version once this PR is merged, since changes from #25 are needed for https://github.com/UN-OCHA/hpc_service/pull/2445

@Pl217 Pl217 merged commit 740307f into develop Nov 3, 2021
@Pl217 Pl217 deleted the attachment-improvements branch November 3, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pl217 Pl217 Pl217 approved these changes

Assignees

@czmj czmj

@Pl217 Pl217

Labels
ready for review All comments have been addressed, and the Pull Request is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@s0 @Pl217 @czmj