Introduce a Project description property and support for it in node #9511
Conversation
fviernau
changed the title
Project description property and support for it in node
fviernau
changed the title
Project description property and support for it in node
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9511 +/- ##
============================================
+ Coverage 67.96% 67.97% +0.01%
Complexity 1290 1290
============================================
Files 249 249
Lines 8810 8813 +3
Branches 915 916 +1
============================================
+ Hits 5988 5991 +3
Misses 2433 2433
Partials 389 389
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| /** | ||
| * The description of project. | ||
| */ | ||
| val description: String = "", |
There was a problem hiding this comment.
I tend to not assign a default value here (and instead assign the empty string in Project.EMPTY), even if that requires more code changes.
But I'm anyway in the context of #9417 preparing a helper function to create projects (with always the correct projectType). Maybe we can postpone this PR until then, so that this change only needs to be done in a single place?
There was a problem hiding this comment.
But we could also do it the opposite way, merge this first and fixup the default argument later.
There was a problem hiding this comment.
Not using a default value primary consideration IMO should be whether we need backwards compatible deserialization. As you didn't mention this explicitly, I want to double check whether your intention is to break this compatability and if so why?
There was a problem hiding this comment.
But we could also do it the opposite way, merge this first and fixup the default argument later.
I prefer to merge this one first, as that other mentioned change also depends on my node refactoring.
There was a problem hiding this comment.
whether we need backwards compatible deserialization.
Yes, we should have that. I overlooked your sentence in the commit message. So let's keep the empty default value.
There was a problem hiding this comment.
that other mentioned change also depends on my node refactoring.
I don't think it does (strictly), as it's a rather small change. But I'm anyway fine with merging this PR first.
| @@ -15,7 +15,7 @@ project: | |||
| type: "Git" | |||
| url: "<REPLACE_URL_PROCESSED>" | |||
| revision: "<REPLACE_REVISION>" | |||
| path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/npm/babel" | |||
| path: "<REPLACE_PATH>" | |||
There was a problem hiding this comment.
Commit message: Please explain why. As the test seems to have worked before, what's the improvement now?
There was a problem hiding this comment.
IIUC all <REPLACE_PATH> pattern are not necessary to make the tests work. But I can explain why regardlessly.
There was a problem hiding this comment.
I'm still not convinced. <REPLACE_...> patterns in general should only be used for stuff that cannot be hard-coded in expected results, that's why the replace mechanism was introduced.
So if we're using <REPLACE_PATH> (or any other <REPLACE_...> pattern) where hard-coding would be possible in the expected result, we should hard-code instead.
There was a problem hiding this comment.
This would be a change in the approach, just consider the REPLACE_DEFINITION_FILE_PATH. That's probably not necessary in any case but we have it. This commit is only about making things (locally) consistent. I'm not against changing the approach per se, but not in this PR / commit.
BTW: The use of these patterns makes it easier to rename files, or to move them.
There was a problem hiding this comment.
The use of these patterns makes it easier to rename files, or to move them.
So that is the argument that convinces me, and that I would have liked to read in the commit message. I'll approve.
| @@ -85,6 +85,11 @@ data class Project( | |||
| */ | |||
| val vcsProcessed: VcsInfo = vcs.normalize(), | |||
|
|
|||
| /** | |||
| * The description of project. | |||
There was a problem hiding this comment.
Do you guys believe empty values should be serialized or omit from serialization?
(Former requires updating all expected results)
There was a problem hiding this comment.
I believe empty / default values should be omitted (for smaller file size).
fviernau
force-pushed
the
feat-model-project-description
branch
from
4d4b53d to
341423a
Compare
Use a default value to ensure backwards compatible de-serialization. Fixes #9443. Signed-off-by: Frank Viernau <[email protected]>
Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
feat-model-project-description
branch
from
341423a to
debd44c
Compare
Signed-off-by: Frank Viernau <[email protected]>
Using the replace pattern for the path is not necessary to make the test pass, analog to other tests, but improves consistency. Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
feat-model-project-description
branch
from
debd44c to
c983dc3
Compare
Add the
descriptionproperty to project's, analog to the one in packages and implement support in all node package managers.Fixes #9443.