codecov-commenter
commented
6 months ago Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 70.9%. Comparing base (
676aae2) to head (b84f7e7).
Closed thejohnfreeman closed 5 months ago
codecov-commenter
commented
6 months ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 70.9%. Comparing base (
676aae2) to head (b84f7e7).
godexsoft
commented
6 months ago @scottschurr I have no objections either way. If moving to .h is significantly easier then that's a valid strategy 👍
I'm wondering however who is it disruptive for? Does anyone but Clio actually consume libxrpl? From Clio's perspective, where we already have everything as .hpp it would look better if rippled's headers were also .hpp but that's very minor.
scottschurr
commented
6 months ago I have no objections either way. If moving to .h is significantly easier then that's a valid strategy 👍 I'm wondering however who is it disruptive for?
That's a fair question, @godexsoft. In terms of disruption I was thinking of folks, like me, who work in the rippled code base. Renaming fewer files means I don't have to retrain that part of my brain. I also frequently have trouble tracing history through renamed files in git. Reducing the number of renamed files will increase the likelihood that looking at history using git continues to work for more files.
But neither of those are killer arguments. It's mostly about my personal convenience. So, yeah, I'm still leaning toward the current approach that @thejohnfreeman has taken.
thejohnfreeman
commented
6 months ago I prefer .hpp to .h too, stylistically, but the .hpp files are in the extreme minority in this codebase, to the point that they are overlooked: they have been mistakenly excluded from the formatting requirements for years.
The next PR is about to rename every file in src/ripple. If we want to adopt .hpp, that would be the time, but I don't feel strongly enough to fight for it.
ximinez
commented
6 months ago Does anyone but Clio actually consume
libxrpl?
Two that I know about are:
thejohnfreeman
commented
5 months ago @Bronek the packaging files have been moved to a separate, internal (Ripple GitLab) project. They were never used outside of Ripple. They drive our internal package pipelines. As a separate project, they can be developed outside of the rippled review and release process. I've already worked with @legleux to ensure that the new project can successfully package this branch.
thejohnfreeman
commented
5 months ago Now that this PR has two approvals, I will work on the finishing steps outlined at the end of the PR description.
This PR implements half of the changes planned for the physical redesign. These changes, described below, are in files touched infrequently by other contributors, so I expect them to be less disruptive or contentious. (The second half of the changes will come in a later PR that I expect to be more contentious.)
Builds/containers, which I've been helping @legleux over the last couple of weeks exfiltrate to a separate project, to decouple its development cadence from that of rippled. As far as I know, only Ripple uses these files, and only to build and test packages.external/by movingsecp256k1anded25519-donnaout ofsrc/. From now on,src/will contain only first-party sources..has the only extension for first-party headers. This could require an importer to change its include paths, but the only affected headers are part ofbeast/{unit_,}test/, so I don't think any other projects are affected (validator-keys-toolis not).Before this PR is merged, we should:
develop. The commit sequence that predates the review should be preserved. Fixes made since the review opened should be merged into earlier commits..git-blame-ignore-revs.