Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle deployment selection inconsistencies #161

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from
Open

fix: handle deployment selection inconsistencies #161

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b29269e
fix: handle deployment selection inconsistencies
rahulyadav-57 Nov 20, 2024
8b8b17b
refactor: abi file filter, extractContractName and file manipulation
rahulyadav-57 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 45 additions & 19 deletions src/components/workspace/BuildProject/BuildProject.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,23 +102,29 @@ const BuildProject: FC<Props> = ({ projectId, contract, updateContract }) => {

const contractsToDeploy = () => {
return projectFiles
.filter((f) => {
const _fileExtension = getFileExtension(f.name || '');
return (
f.path.startsWith(`${activeProject?.path}/dist`) &&
['abi'].includes(_fileExtension as string)
.filter((file) => {
const fileExtension = getFileExtension(file.name || '');
const isAbiFile =
file.path.startsWith(`${activeProject?.path}/dist`) &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fileExtension === 'abi';

if (activeProject?.language === 'func') {
return isAbiFile;
}

const hasTsFile = projectFiles.some(
(f) => f.path === file.path.replace('.abi', '.ts'),
);
return isAbiFile && hasTsFile;
})
.map((f) => {
return {
id: f.id,
name: f.name
.replace('.abi', '')
.replace('tact_', '')
.replace('func_', ''),
path: f.path,
};
});
.map((file) => ({
id: file.id,
name: file.name
.replace('.abi', '')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.replace('tact_', '')
.replace('func_', ''),
path: file.path,
}));
};

const cellBuilder = (info: string) => {
Expand Down Expand Up @@ -519,7 +525,7 @@ const BuildProject: FC<Props> = ({ projectId, contract, updateContract }) => {
setEnvironment(network);
};

const updateSelectedContract = (contract: string) => {
const updateSelectedContract = (contract: string | undefined) => {
setSelectedContract(contract);
updateProjectSetting({
selectedContract: contract,
Expand Down Expand Up @@ -638,9 +644,29 @@ const BuildProject: FC<Props> = ({ projectId, contract, updateContract }) => {
if (activeProject?.network) {
setEnvironment(activeProject.network);
}
if (activeProject?.selectedContract) {
setSelectedContract(activeProject.selectedContract);
deployForm.setFieldsValue({ contract: activeProject.selectedContract });
const selectedABIPath = activeProject?.selectedContract;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • These strings are repeated in other parts of code.
  • The replace goes over the whole string, and .abi can be any part of it. For example, .abi, .abi.foo.cpp or h.abibi.txt.


let _selectedContract: string | undefined = selectedABIPath;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is used. _ convention is for variables that are unused, or for private class fields.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable doesn't have to be mutable. By abstracting the logic into getSelectedContract() it could be just a return.


if (activeProject.language === 'tact') {
const scriptFile = projectFiles.find(
(file) => file.path === correspondingScriptPath,
);
const hasValidScriptFile =
scriptFile &&
projectFiles.find((file) => file.path === selectedABIPath);

_selectedContract = hasValidScriptFile ? selectedABIPath : undefined;
}

deployForm.setFieldsValue({
contract: _selectedContract,
});

updateSelectedContract(_selectedContract);
}
const handler = (
event: MessageEvent<{
Expand Down