fix: handle deployment selection inconsistencies #161
Conversation
| .filter((file) => { | ||
| const fileExtension = getFileExtension(file.name || ''); | ||
| const isAbiFile = | ||
| file.path.startsWith(`${activeProject?.path}/dist`) && |
There was a problem hiding this comment.
Please encapsulate all the logic related to filenames into a separate file.
If there is another place in the code that would need to check if the file is an ABI-file, it would be found there.
The underlying problem is that file paths are usually untyped, and it's unsafe to work with them with String methods. While working with paths as if they were Strings is not avoidable (that's the OS interface to work with files after all), it's possible to at least give proper attention to changes of unsafe code, if it's encapsulated in a separate file.
| .map((file) => ({ | ||
| id: file.id, | ||
| name: file.name | ||
| .replace('.abi', '') |
There was a problem hiding this comment.
These strings are repeated several times over the code base, and in case there'll be a change to them, there'll be no way to ensure modification is consistent.
It'd be better to assign this replace action a name and encapsulate all the related strings in a single file.
| if (selectedABIPath) { | ||
| const correspondingScriptPath = selectedABIPath.replace('.abi', '.ts'); | ||
|
|
||
| let _selectedContract: string | undefined = selectedABIPath; |
There was a problem hiding this comment.
This variable is used. _ convention is for variables that are unused, or for private class fields.
| const selectedABIPath = activeProject?.selectedContract; | ||
|
|
||
| if (selectedABIPath) { | ||
| const correspondingScriptPath = selectedABIPath.replace('.abi', '.ts'); |
There was a problem hiding this comment.
- These strings are repeated in other parts of code.
- The replace goes over the whole string, and
.abican be any part of it. For example,.abi,.abi.foo.cpporh.abibi.txt.
| if (activeProject?.selectedContract) { | ||
| setSelectedContract(activeProject.selectedContract); | ||
| deployForm.setFieldsValue({ contract: activeProject.selectedContract }); | ||
| const selectedABIPath = activeProject?.selectedContract; |
There was a problem hiding this comment.
Abbreviations in camel case are handled as regular words.
Here's a review of the reasoning behind it.
The most important part there is that automated tools can only work if rules do not depend on semantics. For example, if there would be a file that exports selectedABIPath, its name by Node.js file naming standards would be selected-a-b-i-path, and linter would enforce that.
| if (selectedABIPath) { | ||
| const correspondingScriptPath = selectedABIPath.replace('.abi', '.ts'); | ||
|
|
||
| let _selectedContract: string | undefined = selectedABIPath; |
There was a problem hiding this comment.
This variable doesn't have to be mutable. By abstracting the logic into getSelectedContract() it could be just a return.
|
This is open for 3 weeks without progress. Should we close it? |
|
I was working on Misti support, so it was delayed. I'll resolve it soon. |
Resolves #49