Conversation
Thomas1664
left a comment
There was a problem hiding this comment.
I know that this is still a draft.
In general you should be careful with adding const to local variables because this might avoid moving in some cases
| namespace vcpkg::Lint | ||
| { | ||
|
|
||
| constexpr StringLiteral VERSION_RELAXED = "version"; |
There was a problem hiding this comment.
| constexpr StringLiteral VERSION_RELAXED = "version"; | |
| static constexpr StringLiteral VERSION_RELAXED = "version"; |
The others as well.
There was a problem hiding this comment.
|
In general, it might be possible to parallelize linting as well if there weren't any calls to |
Linting all ports currently takes 140 ms. I don't thing there is any need for performance improvements. |
# Conflicts: # src/vcpkg/base/messages.cpp
# Conflicts: # locales/messages.en.json
vicroms
left a comment
There was a problem hiding this comment.
Some parts of this PR can be made unit-testable with a few tweaks.
# Conflicts: # azure-pipelines/end-to-end-tests-dir/format-manifest.ps1 # azure-pipelines/end-to-end-tests-dir/versions.ps1
Suggestions from @vicroms
|
@vicroms I have added unit tests |
# Conflicts: # src/vcpkg/commands.add-version.cpp
|
I think we can do both, the post build check can warn user, and we can expand this flag to something like |
|
I think you don't need a new flag for that. Simply use the |
|
Yes, I will create a PR against this branch once I tested. |
|
People use different ways to install usage, should vcpkg provide a unified |
|
I think the current situation is fine. |
# Conflicts: # locales/messages.json # src/vcpkg/base/messages.cpp
# Conflicts: # locales/messages.json
|
@autoantwort Any news from your linter? |
|
No, I am done and wait for review from @vicroms |
|
@vicroms @Neumann-A @Thomas1664 |
Thomas1664
left a comment
There was a problem hiding this comment.
My earlier comment to add static to some string literals is still unfixed but like my new comment not a big deal.
I also noticed the use of std::string::size_type instead of std::size_t or size_t which is also not a problem.
|
@autoantwort Are you going to fix the conflicts? |
|
I can fix them, but they don't prevent this PR from being reviewed. |
# Conflicts: # include/vcpkg/base/messages.h # src/vcpkg/base/messages.cpp
|
@Thomas1664 this PR is ready for review by @vicroms from your side? |
The vcpkg team reviews PRs independently from community feedback such as my review comments. |
# Conflicts: # include/vcpkg/base/messages.h # src/vcpkg/base/messages.cpp
# Conflicts: # src/vcpkg/commands.add-version.cpp
# Conflicts: # src/vcpkg/commands.cpp
|
Any news? |
I think this keeps getting skipped over because we need to sell a concrete proposal of what this command actually does to our PM team and management, and we aren't getting time to reverse engineer what that looks like from this PR in order to do so. |
All you need is time? |
We need like a spec of what the command is supposed to do |
Adds command
x-lint-portthat checks for common port issues and the usage of deprecated functions. The command is also able to fix the most of these issues.