WIP: Repository class refactoring #95

arthurmco · 2025-01-27T14:11:03Z

Refactor the repository class based on @dhilst 's modeling

viniciusferrao · 2025-01-27T14:19:24Z

src/services/repo.cpp

+        return file;
+    } catch (Glib::FileError& e) {
+        throw repository_exception(
+            std::format("Could not load repository file {} ({})",


fmt::format.

viniciusferrao · 2025-01-27T14:19:39Z

src/services/repo.cpp

+
+void ELRepoFile::read()
+{
+    printf("reading\n");


viniciusferrao · 2025-01-27T14:20:17Z

src/services/repo.cpp

+    for (const auto& r : m_repositories) {
+        std::print("repo found '{}'\n", r.group);
+        std::print("\tname: '{}'\n", r.name);
+        std::print("\tbaseurl: '{}'\n", r.baseURL.value_or("null"));
+        std::print("\tmetalink: '{}'\n", r.metalink.value_or("null"));
+        std::print("\tenabled: {}\n", r.enabled);
+        std::print("\tgpg: (check: {}, key: {})\n", r.gpgcheck, r.gpgkey);
+    }


Is this debug? If yes add #ifndef NDEBUG. Also we are using fmt::format instead and use the LOG_DEBUG directive.

This was glibmm test code I forgot to remove.

include/cloysterhpc/services/repo.h

viniciusferrao · 2025-01-27T14:25:50Z

src/services/repo.cpp

+        m_repositories.push_back(std::move(repo));
+    }
+
+    for (const auto& r : m_repositories) {


I would avoid using only r as variable name. Usually we use it for well defined counters, like: i, j and k. Or for iterators, like it.

This is only code for toying with glibmm

In real life, this would not be like this

(This comment applies to every printf/fmt::format comment above)

viniciusferrao · 2025-01-27T14:27:43Z

Refactor the repository class based on @dhilst 's modeling

The modeling was done by the three of us. You included!

dhilst · 2025-01-27T14:30:13Z

include/cloysterhpc/services/repo.h

+/**
+ * Repository file class
+ */
+template <class Repo> class RepoFile {
+protected:
+    std::filesystem::path m_path;
+    std::vector<Repo> m_repositories;


You are mixing up "repository" and File class

Remove the Repo type parameter

Rename RepoFile to File

Remove m_repositories member

This class only wraps file I/O, it must not know anything about repositories

Like this?

class File { protected: std::filesystem::path m_path; public: RepoFile(const std::filesystem::path& path) : m_path(path) {} virtual void read() {} virtual void write() {} };

(Me including the repositories list there was a mistake... I already removed it)

Partially, there will be an entity named something like "File" on the namespace "cloysterhpc" that will handle basic file operations. Open, close, write, read, chmod. There's a lot of that on glib and we may use them:

https://gnome.pages.gitlab.gnome.org/glibmm/classGio_1_1File.html
https://gnome.pages.gitlab.gnome.org/glibmm/group__FileUtils.html
https://gnome.pages.gitlab.gnome.org/glibmm/group__UriUtils.html

I do believe that GIO will solve almost everything.

dhilst · 2025-01-27T14:34:15Z

include/cloysterhpc/services/repo.h

+class ELRepoFile : public RepoFile<ELRepo> {
+private:
+    Glib::RefPtr<Glib::KeyFile> loadFile();
+
+public:
+    ELRepoFile(const std::filesystem::path& path)
+        : RepoFile(path)
+    {
+    }
+
+    void read() override;
+    void write() override;
+};


We can have an abstract RepoFile which extends the File, adds the m_repositories generic member, and with parse unparse methods, read is super read followed by parse and write is unparse followed by super write

std::filesystem::path is convertible to std::string or const char*. We must use it. It's the right API.

However the file class that be dependent on std::filesystem::path and not the derived one.

Just to be clear

parse loads data from the file into the m_repositories member

unparse loads the m_repositories member into the file

Also, @arthurmco, add overloads to parse and unparse which I/O into istringstream ostringstream instead of directly into the file so we can test without touching the disks. I don't know if this is possible because I don't know the interface of glibmm if it can load/unload keyfiles from strings, can it?

This is possible; glibmm can load keyhole data from a string.

dhilst · 2025-01-27T14:52:35Z

include/cloysterhpc/services/repo.h

+class repository_exception : public std::runtime_error {
+public:
+    explicit repository_exception(const std::string& msg)
+        : std::runtime_error(msg)
+    {
+    }
+};


@viniciusferrao opinions on this? if we'll not be handling exceptions is there any point in creating our own exceptions?

I think this is just a WIP. But not sure. But it is a good practice to specialize the throw: https://isocpp.org/wiki/faq/exceptions#what-to-throw

This is just a WIP

dhilst · 2025-01-27T17:03:02Z

src/services/repo.cpp

+        auto metalink = (file->has_key(repogroup, "metalink"))
+            ? std::make_optional(file->get_string(repogroup, "metalink"))
+            : std::nullopt;


Optionals are really cool. Add a free function for reading an optional from keyfiles

I'm an idiot. I have too much difficulty reading ternary operators. That's a technical debt in my mind.

The std::nullopt should be inside a std::make_optional? I don't know.

It is okay, you can read this like this if (file->has_key(repogroup, "metalink")) { metalink = std::make_optional(file->get_string(repogroup, "metalink")) } else { metalink = std::nullopt; }. metalink is an optional set to the metalink property if it exists in the file, or set to nullopt if not. This is pretty general, we'll want to use std::optional to represent optional values in the conf file, so I'm suggesting that we create a function for that

inline std::optional<std::string> get_optional_string( const KeyFile& fil, const std::string& group, const std::string& key) { if (fil->has_key(group, key)) { return fil->get_string(group, key); } return std::nullopt; }

The optional is got with the .value() accessor, right?

yes, but glib keyfile is not using optional here, I guess the interface is older than std::optional

yes, but glib keyfile is not using optional here, I guess the interface is older than std::optional

Yes it is. But we are talking about the same issue? That our API shuould provide access with std::optional and inside the class we encapsulate that? If that's the case yes I do agree.

Are you saying for us to extend glibmm file interface and add this as a member function?

dhilst · 2025-01-30T13:48:31Z

src/services/repo.cpp

+std::vector<ELRepo> ELRepoFile::parse(const std::stringstream& ss)
+{
+    m_file = Glib::KeyFile::create();
+    m_file->load_from_data(ss.str().c_str());
+    return this->parseData();
+}


Cool, now

Add m_repositories member as a vector of ELRepo with a getter

The getter should load the data into the member if the member is empty and return at the end

Inside of ELRepo?

Inside ELRepoFile

dhilst · 2025-01-30T13:50:52Z

include/cloysterhpc/services/repo.h

+struct ELRepo {
+    std::string group;
+    std::string name;
+    std::optional<std::string> baseURL;


dhilst · 2025-01-30T13:51:27Z

include/cloysterhpc/services/repo.h

+    void unparse( const std::vector<ELRepo>& repositories );
+    void unparse( const std::vector<ELRepo>& repositories, std::stringstream& ss );


Suggested change

void unparse( const std::vector<ELRepo>& repositories );

void unparse( const std::vector<ELRepo>& repositories, std::stringstream& ss );

void unparse(const std::vector<ELRepo>& repositories);

void unparse(const std::vector<ELRepo>& repositories, std::stringstream& ss);

dhilst · 2025-01-30T14:04:20Z

src/services/repo.cpp

+    auto get_optional_string
+        = [this](auto group, auto key) -> std::optional<Glib::ustring> {
+        return m_file->has_key(group, key)
+            ? std::make_optional(m_file->get_string(group, key))
+            : std::nullopt;
+    };


I don't see why this captures this, anyway, move it to functions.cpp

dhilst

The API looks good, there are a few points

Make sure to remove the prints
Rename the ELReo to ELCloneRepo
Remove the PR from draft

viniciusferrao

Another issue that I've observed. The virtual classes should be on its own files. This needs to be fixed.

viniciusferrao · 2025-01-30T13:54:28Z

src/services/repo.cpp

+
+std::vector<ELRepo> ELRepoFile::parse()
+{
+    this->read();


Do we need "this"?

viniciusferrao · 2025-01-30T13:56:12Z

src/services/repo.cpp


        if (name.empty()) {
            throw repository_exception(std::format(
-                "Could not load repo name from repo '{}' at file {}",
+                "Could not load repo name from repo '{}' at m_file {}",


This is more than a DEBUG message for the developer than to the user. For the user it should be something more friendly.

What do you suggest?

viniciusferrao · 2025-01-31T14:55:44Z

include/cloysterhpc/functions.h

@@ -164,6 +166,9 @@ std::string findAndReplace(const std::string_view& source,
 */
 void copyFile(std::filesystem::path source, std::filesystem::path destination);

+std::optional<Glib::ustring> readKeyfileString(Glib::RefPtr<Glib::KeyFile> file,
+    const std::string_view& group, const std::string_view& key);


std:string_view is already a pointer to a string. It should not be passed as const ref.

I didn't know that. I thought it was pointer+length. It will be fixed

viniciusferrao · 2025-01-31T14:56:36Z

include/cloysterhpc/services/repo.h

 /**
 * Repository file class
 *
 * This class should parse the repository data
 */
 class ELRepoFile : GenericFile {
-private:    
+private:
    Glib::RefPtr<Glib::KeyFile> m_file;

    std::vector<ELRepo> parseData();
    void unparseData(const std::vector<ELRepo>&);


Missing the variable name.

The variable name is not needed in the declaration and the context is obvious, but I will fixed because of conventions

viniciusferrao · 2025-02-01T02:03:58Z

src/functions.cpp

@@ -351,4 +351,12 @@ void copyFile(std::filesystem::path source, std::filesystem::path destination)
    }
 }

+std::optional<Glib::ustring> readKeyfileString(Glib::RefPtr<Glib::KeyFile> file,
+    const std::string_view& group, const std::string_view& key)


No need to be const ref.

See this comment

sonarqubecloud · 2025-02-04T16:51:47Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

WIP: Repository class refactoring

a01df8c

arthurmco requested a review from dhilst January 27, 2025 14:11

arthurmco self-assigned this Jan 27, 2025

viniciusferrao requested changes Jan 27, 2025

View reviewed changes

dhilst reviewed Jan 27, 2025

View reviewed changes

arthurmco added 2 commits January 30, 2025 08:31

Add parse() and unparse() methods, fix more things

6fe6003

Add stringstream read specialization, as suggested by @dhilst

99f79ce

dhilst reviewed Jan 30, 2025

View reviewed changes

arthurmco added 3 commits January 31, 2025 09:38

Move some common functions to functions.h

3f0f219

Remove this->

37f8b60

Fix repo class

ab4ba11

dhilst approved these changes Jan 31, 2025

View reviewed changes

dhilst requested changes Jan 31, 2025

View reviewed changes

viniciusferrao requested changes Feb 1, 2025

View reviewed changes

arthurmco added 2 commits February 3, 2025 10:30

Fix const ref and declaration name

25df8ed

Fix naming

de966e5

arthurmco requested a review from dhilst February 4, 2025 13:44

arthurmco marked this pull request as ready for review February 4, 2025 13:45

arthurmco added 2 commits February 4, 2025 12:37

Rename required files

4895e2e

fixup! Rename required files

a834f94

		void unparse( const std::vector<ELRepo>& repositories );
		void unparse( const std::vector<ELRepo>& repositories, std::stringstream& ss );

WIP: Repository class refactoring #95

Are you sure you want to change the base?

WIP: Repository class refactoring #95

Conversation

arthurmco commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusferrao commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusferrao Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

dhilst Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhilst Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viniciusferrao Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

dhilst Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

viniciusferrao Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhilst Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhilst left a comment

Choose a reason for hiding this comment

viniciusferrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 4, 2025

Quality Gate passed

arthurmco commented Jan 27, 2025 •

edited

Loading

viniciusferrao Jan 28, 2025 •

edited

Loading

dhilst Jan 27, 2025 •

edited

Loading

dhilst Jan 27, 2025 •

edited

Loading

viniciusferrao Jan 27, 2025 •

edited

Loading

dhilst Jan 28, 2025 •

edited

Loading

viniciusferrao Jan 28, 2025 •

edited

Loading

dhilst Jan 30, 2025 •

edited

Loading