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.

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

tools: utilize environment specific files for vsymlink #21360

Merged
merged 5 commits into from
Apr 27, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Apr 27, 2024

To keep the review simpler this PR does an update of the current state prior to an extraction. Also, this can be good to have in the history.

The followup would be a structural update that allows for better organization and maintainability:

├── tools
│   ├── vsymlink
│   │   ├── vsymlink.v
│   │   ├── vsymlink_nix.c.v
│   │   └── vsymlink_windows.c.v
│   ├── ...

edit: Splitting the changes runs into some issues. To save some time, I'd directly include the extraction. The conceptual idea of the beforehand simplification would remain part of this PRs history.

@ttytm ttytm changed the title tools: update vsymlink before to extraction tools: utilize environment specific files for vsymlink Apr 27, 2024
@spytheman
Copy link
Member

The tool is 210 lines on master, and is not going to change much in functionality. Only bug fixes for it will happen.

I do not see the value of splitting it into several files 🤔 .

@ttytm
Copy link
Member Author

ttytm commented Apr 27, 2024

The tool is 210 lines on master, and is not going to change much in functionality. Only bug fixes for it will happen.

I do not see the value of splitting it into several files 🤔 .

It would make a direct distinction between the platform commands, and therefore the use of all the $if windows in the current file obsolete. Alternatively, if it's preferred to keep it in one file I'd just removing the anti-pattern of wrapping whole function bodies in if statements and reduce unnecessary nesting.

@spytheman
Copy link
Member

All true, and yet, I feel a bit uneasy about splitting a 210 line file... Anyway that is a personal feeling, I'll get over it.

Good work.

@spytheman spytheman merged commit 0e711f9 into vlang:master Apr 27, 2024
55 checks passed
@ttytm
Copy link
Member Author

ttytm commented Apr 27, 2024

Thanks 👍

@ttytm ttytm deleted the tools/symlink branch April 27, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants