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

refactor: add index method for StacksEpochId #5350

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

obycode
Copy link
Contributor

@obycode obycode commented Oct 21, 2024

This allows us to cleanup some magic numbers when accessing epochs in a list and make the expected behavior more clear to the reader.

This allows us to cleanup some magic numbers when accessing epochs in a
list and make the expected behavior more clear to the reader.
jcnelson
jcnelson previously approved these changes Oct 21, 2024
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM assuming CI passes

The list of epochs passed in here is not complete, so we cannot use the
standard epoch indices for it.
@obycode
Copy link
Contributor Author

obycode commented Oct 21, 2024

There was once place acting on a list of epochs where the list was incomplete, so the standard indices shouldn't be used. This fix should resolve the failing tests.

@kantai
Copy link
Member

kantai commented Oct 21, 2024

I think it'd be good to try to clean up the magic numbers and accessors used in epoch lists, but I'm kind of wary of encoding the indexes, even though it probably is perfectly safe (because there's no legal epoch ordering where 2.4 doesn't exist, but 2.5 does).

An alternative that I think would have some of the same benefits would be a wrapper struct...

pub struct EpochList (Vec<StacksEpoch>)

And then defining:

impl Index<&StacksEpochId> for EpochList {
  type Output = StacksEpoch;
  fn index(&self, index: &StacksEpochId) -> &StacksEpoch {
     self.get(index).unwrap()
  }
}

impl EpochList {
  fn get(&self, index: &StacksEpochId) -> Option<&StacksEpoch> {
     self.0.get(StacksEpoch::find_by_epoch_id(&self.0, index)?)
  }
}

Then, we could do things like epochs[StacksEpochId::Epoch30] for panicking uses, and epochs.get(Epoch30) for non-panicking uses

@obycode
Copy link
Contributor Author

obycode commented Oct 21, 2024

Sure, that does seem nice. Likely a little slower, but I don't think we'd use it anywhere that is performance critical.

@obycode obycode marked this pull request as draft October 22, 2024 18:06
Wrapper struct to make the accessing of the individual epochs easier and
standardized.
@obycode obycode marked this pull request as ready for review October 23, 2024 17:30
@obycode obycode requested a review from kantai October 23, 2024 17:30
@obycode obycode marked this pull request as draft October 23, 2024 21:49
@obycode obycode marked this pull request as ready for review October 24, 2024 12:53
@obycode obycode requested a review from jcnelson October 24, 2024 12:53
@obycode
Copy link
Contributor Author

obycode commented Oct 24, 2024

Ok, re-refactored with pub struct EpochList (Vec<StacksEpoch>)

stackslib/src/core/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This looks good to me-- it definitely improves the legibility of the epochs lists! Just a couple comments.

@obycode obycode requested a review from kantai November 6, 2024 03:36
kantai
kantai previously approved these changes Nov 7, 2024
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