Skip to content

Conversation

@mwinn2
Copy link
Contributor

@mwinn2 mwinn2 commented Nov 29, 2019

WIP: moving digits into Base
@aphecetche : I don't manage to get the linking properly done, sorry, but the easiest is, if you have a look, I think (with my cmake clumsiness it takes otherwise quite some time).

@mwinn2 mwinn2 requested a review from a team as a code owner November 29, 2019 09:33
@aphecetche
Copy link
Collaborator

@mwinn2 I've opened a PR to your branch : mwinn2#7 to show what I had to change to make it compile and link correctly. There's a question though on what to do with the timestamp.

@sawenzel
Copy link
Collaborator

Formatting is failing. Can we go on here?

@aphecetche
Copy link
Collaborator

@mwinn2 could you fix the clang-format stuff ?

@sawenzel
Copy link
Collaborator

sawenzel commented Jan 7, 2020

Seems to be ok now. @aphecetche Can you approve the PR?

@sawenzel sawenzel requested a review from aphecetche January 7, 2020 17:06
@shahor02
Copy link
Collaborator

shahor02 commented Jan 7, 2020

Just out of curiosity: is there a reason to move the Digit to {{Detectors/MUON/MCH/Base}} and not to more natural {{DataFormats/Detectors/MUON/MCH}} ?

@sawenzel
Copy link
Collaborator

sawenzel commented Jan 8, 2020

It probably makes sense to systematically collect these data structures under "DataFormats".
It would be quite natural doing it now but we can also do it later (@aphecetche, @mwinn2 your call). The same should be done for other detectors.

@aphecetche
Copy link
Collaborator

I would prefer we merge this one and move the relevant structures to their "final" destination on a different PR.

Copy link
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

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

OK, merging with squashing

@shahor02 shahor02 merged commit 4433ee1 into AliceO2Group:dev Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants