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

config: use FactSet data pulled in April #225

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

AlexAxthelm
Copy link
Collaborator

@AlexAxthelm AlexAxthelm commented Apr 23, 2024

Includes new ISINs, as well as update Fund data as requested from a CH user, for the PA2024CH project.

includes new ISINs for PA2024CH project
@AlexAxthelm AlexAxthelm requested a review from cjyetman as a code owner April 23, 2024 08:40
Copy link
Contributor

@Antoine-Lalechere Antoine-Lalechere left a comment

Choose a reason for hiding this comment

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

lgtm

@cjyetman
Copy link
Member

Thanks. @jdhoffa is going to check with all relevant people to make sure they understand the consequences of changing the data midstream in a COP project, and I will approve/merge once I get a positive feedback from that.

@cjyetman
Copy link
Member

ack, sorry... for 2023Q4 this is fine to do now

@AlexAxthelm
Copy link
Collaborator Author

Thanks. @jdhoffa is going to check with all relevant people to make sure they understand the consequences of changing the data midstream in a COP project, and I will approve/merge once I get a positive feedback from that.

I think this is still relevant, since this change is specifically intended to resolve issues with PA2024CH.

@AlexAxthelm AlexAxthelm requested a review from jdhoffa April 23, 2024 09:06
@cjyetman cjyetman self-requested a review April 23, 2024 09:06
@cjyetman
Copy link
Member

actually, I think we should consider this more... I think it might be prudent to create a new 2023Q4_PA2024CH config because it's clear that a situation can easily occur where the config for a COP project is different than what we want for GENERAL. @jdhoffa can we discuss a strategy for this

@jdhoffa
Copy link
Member

jdhoffa commented Apr 23, 2024

@Antoine-Lalechere and @AlexAxthelm thanks for this!
The PR LGTM and I am 99% sure we are going to go this direction and merge this, but would like to hold off until we get a final decision based on all associated risks (see comment here: https://dev.azure.com/RMI-PACTA/2DegreesInvesting/_workitems/edit/10208#30762716)

@jdhoffa
Copy link
Member

jdhoffa commented Apr 23, 2024

actually, I think we should consider this more... I think it might be prudent to create a new 2023Q4_PA2024CH config because it's clear that a situation can easily occur where the config for a COP project is different than what we want for GENERAL. @jdhoffa can we discuss a strategy for this

CJ, to this end I have created a new issue here: #226

Given that GENERAL has not yet been released, and we have no other COP projects, I am tempted to deprioritize this for now, but I think this feature will be something we should tackle if ever we intend to do another COP project this year (e.g. using 2023Q4 data)

@jdhoffa jdhoffa changed the title Use new Factset data Use FactSet data pulled in April Apr 23, 2024
@jdhoffa jdhoffa changed the title Use FactSet data pulled in April update: use FactSet data pulled in April Apr 23, 2024
@jdhoffa jdhoffa changed the title update: use FactSet data pulled in April config: use FactSet data pulled in April Apr 23, 2024
@jdhoffa
Copy link
Member

jdhoffa commented Apr 23, 2024

Based on this decision: https://dev.azure.com/RMI-PACTA/2DegreesInvesting/_workitems/edit/10208#30766213

We are good to go!

@AlexAxthelm feel free to merge when ready.

@AlexAxthelm AlexAxthelm merged commit 5a96cc3 into main Apr 23, 2024
1 check passed
@AlexAxthelm AlexAxthelm deleted the feat/update-fs-to-use-new-isins branch April 23, 2024 13:54
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.

4 participants