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

Trace to events #80

Merged
merged 21 commits into from
Nov 30, 2023

Conversation

Modularius
Copy link
Contributor

@Modularius Modularius commented Nov 6, 2023

Closes #49

@DanNixon
Copy link
Member

DanNixon commented Nov 7, 2023

Is there no way this PR can be split up? 7k lines is way too big for a single PR.

@Modularius
Copy link
Contributor Author

Yes, I can submit the trace-to-pulses crate separately.

@DanNixon
Copy link
Member

DanNixon commented Nov 7, 2023

A few things that would form a starting point of making this more reviewable:

  • Move the code from the trace-to-pulses crate into trace-to-events (the former is just support code for the latter, so I see no real reason for the extra crate)
  • Change the visibility modifiers from pub to pub (crate) (the compiler will then issue a warning when you have code that is not actually used)
  • The unused code can then be removed (this is not to say that it is not valuable, just that it does not yet belong on main and would be better kept in a branch until it is required)

@DanNixon
Copy link
Member

DanNixon commented Nov 7, 2023

Yes, I can submit the trace-to-pulses crate separately.

That will not really help, that crate is the vast majority of the code in this PR.

Copy link
Member

@DanNixon DanNixon left a comment

Choose a reason for hiding this comment

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

An initial look over. There are more places with style issues that I did not comment on.

trace-to-events/Cargo.toml Show resolved Hide resolved
trace-to-events/README.md Outdated Show resolved Hide resolved
trace-to-events/src/main.rs Outdated Show resolved Hide resolved
trace-to-events/src/main.rs Outdated Show resolved Hide resolved
trace-to-events/src/main.rs Outdated Show resolved Hide resolved
trace-to-events/src/pulse_detection/datatype/savable.rs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
trace-to-events/src/parameters.rs Outdated Show resolved Hide resolved
trace-to-events/src/parameters.rs Outdated Show resolved Hide resolved
trace-to-events/src/main.rs Outdated Show resolved Hide resolved
@DanNixon DanNixon merged commit 81165ae into STFC-ICD-Research-and-Design:main Nov 30, 2023
9 checks passed
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.

Event formation MVP
2 participants