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 produce_eko_photon to evolven3fit_new #1778

Merged
merged 6 commits into from
Jul 19, 2023
Merged

Conversation

niclaurenti
Copy link
Contributor

PR for adding produce_eko_photon in evolven3fit_new

@niclaurenti niclaurenti changed the title Import changes from produce_eko_photon Add produce_eko_photon to evolven3fit_new Jul 17, 2023
@niclaurenti niclaurenti marked this pull request as ready for review July 18, 2023 09:40
Copy link
Contributor

@andreab1997 andreab1997 left a comment

Choose a reason for hiding this comment

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

It looks ok but please instead of branching in construct_eko_cards, just make another function for the photon. At the end in construct_eko_cards you are doing a lot of different things so it is probably better to have another function. All the things that instead are in common you can put in yet another function to be called in both cases.

@niclaurenti
Copy link
Contributor Author

It looks ok but please instead of branching in construct_eko_cards, just make another function for the photon. At the end in construct_eko_cards you are doing a lot of different things so it is probably better to have another function. All the things that instead are in common you can put in yet another function to be called in both cases.

Actually the branching produce_eko_photon happens only in three points. I did it like this exactly because there were common blocks of code that I had to copy paste, and I didn't want to add a lot of functions for those blocks

@scarlehoff
Copy link
Member

I agree with @andreab1997 I think it is better to have different functions (and then lift some of the common functionality to wrappers) rather than so many branches.

@niclaurenti
Copy link
Contributor Author

@andreab1997 @scarlehoff Now I have separated the functions construct_eko_cards and construct_eko_photon_cards moving some code in new functions in order not to repeat some common blocks.
Tell me if you want more changes

Copy link
Contributor

@andreab1997 andreab1997 left a comment

Choose a reason for hiding this comment

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

Good for me!

@niclaurenti niclaurenti merged commit 08d8921 into master Jul 19, 2023
@niclaurenti niclaurenti deleted the produce_eko_photon_new branch July 19, 2023 10:32
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