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 and improve code #17

Merged
merged 12 commits into from
Nov 15, 2024
Merged

Refactor and improve code #17

merged 12 commits into from
Nov 15, 2024

Conversation

jcelerier
Copy link
Collaborator

@jcelerier jcelerier commented Oct 27, 2024

  • Fix that nothing would build if including puara-utils in multiple files
  • Put Calibration code in its own file so that code that does not use eigen does not have to install it
  • Add missing default initialization in many places
  • Add CI
  • Remove some unneeded constructors - in modern C++ if a class looks like e.g.
class Wrap
{
public:
  double min{};
  double max{};
  void f() { blabla; }
};

one can simply create it like:

Wrap w1{-100., 456.};

or even better

Wrap w2{.min = -100., .max = 456.};

C++20 also allows to use () instead of {} for this but I think it is better to use {} as it will refuse to do any conversion that would loose precision unlike () which happily does a ton of implicit conversions)

@edumeneses
Copy link
Member

Thanks for that work @jcelerier.

We can merge as is if it's working (I didn't test it).

However, I'm having second thoughts about creating a utils folder and separating the classes as we did with the descriptors. What do you think?

@jcelerier
Copy link
Collaborator Author

yes, great idea ! I'll do that

@jcelerier
Copy link
Collaborator Author

Ok, did a much more thorough cleanup and adapted the library structure to match more closely current expectations and standards in the C++ community

@jcelerier jcelerier force-pushed the refactor/split_utils branch 5 times, most recently from ccaeb07 to 43cefa7 Compare October 28, 2024 08:29
@jcelerier jcelerier force-pushed the refactor/split_utils branch from 43cefa7 to a8f13cd Compare October 28, 2024 08:36
@jcelerier
Copy link
Collaborator Author

Ok, ci builds on Mac/Win/Linux I think we can safely merge :)
I noticed in the refactor that some functions disappeared though, added a few fixme in the code to bring them back to life @samamou

Copy link
Member

@edumeneses edumeneses left a comment

Choose a reason for hiding this comment

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

Looks good.

I fixed 1 silly .gitignore conflict

@jcelerier
Copy link
Collaborator Author

ty!

@jcelerier jcelerier force-pushed the refactor/split_utils branch from 7b40d47 to 1a53061 Compare November 12, 2024 12:47
@edumeneses
Copy link
Member

@jcelerier and @samamou , I noticed the refactored files don't have the latest editor name.

Perhaps adding you/them to the head comments on each descriptor/utils file is a good idea?

@jcelerier
Copy link
Collaborator Author

hmm to be honest my take on this is that this information is part of git history anyways - adding it to the source files is not adding much value to me.
Likewise for the license file, usually at most nowadays we just add SPDX identifiers: https://spdx.github.io/spdx-spec/v2.3/using-SPDX-short-identifiers-in-source-files/

@edumeneses
Copy link
Member

@jcelerier , do we wait for @samamou 's review?

@samamou
Copy link
Member

samamou commented Nov 12, 2024

I think i will need to check for the functions that got removed when we were refactoring, I have some time tomorrow @edumeneses

@samamou samamou requested a review from edumeneses November 14, 2024 14:34
@jcelerier
Copy link
Collaborator Author

build is broken :^)

@jcelerier
Copy link
Collaborator Author

all is green, ACK for me to merge ! congrats :)

@jcelerier
Copy link
Collaborator Author

Edu is it good for you ?

@edumeneses edumeneses merged commit 33441a5 into development Nov 15, 2024
3 checks passed
@jcelerier jcelerier deleted the refactor/split_utils branch November 15, 2024 23:25
@jcelerier
Copy link
Collaborator Author

\o/

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