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

TimeRef is not a singleton anymore. #2396

Merged
merged 3 commits into from
Mar 27, 2023

Conversation

Klaim
Copy link
Member

@Klaim Klaim commented Mar 22, 2023

No description provided.

@Klaim
Copy link
Member Author

Klaim commented Mar 23, 2023

Note: I moved the TimeRef type in it's own header/cpp because I initially wanted to have it in the Context but turns out that if my understanding is correct, it is not necessary. I can move it back into validate.hpp/cpp if you prefer.

#include <chrono>
#include <string>

namespace validate
Copy link
Member

Choose a reason for hiding this comment

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

Should we scope that with mamba::? I've started following that convention with mamba::solv, mamba::util...

Copy link
Member

Choose a reason for hiding this comment

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

+1 on scoping inner namespaces with mamba::

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the namespace that is originally in validate.hpp/cpp 😬
If that's ok with yall I'll change it to mamba::validate in a separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

See: #2411

#include <chrono>
#include <string>

namespace validate
Copy link
Member

Choose a reason for hiding this comment

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

+1 on scoping inner namespaces with mamba::

{
}

TimeRef::~TimeRef() = default;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can rely on destructor automatic generation and remove this line and its declaration in the header.

@@ -711,11 +711,11 @@ namespace validate
RootImpl root(root1_json);

// expiration is set to now+3600s in 'sign_root'
TimeRef::instance().set(utc_time_now());
EXPECT_FALSE(root.expired());
auto time_ref = TimeRef{ utc_time_now() };
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking: TimeRef time_ref;

@Klaim Klaim force-pushed the klaim/context-not-singleton branch from 05630ac to 7606275 Compare March 24, 2023 14:11
@JohanMabille JohanMabille merged commit 1b80aca into mamba-org:main Mar 27, 2023
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.

None yet

3 participants