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

properly fixing vscode project. plus some extra comments. #592

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roymacdonald
Copy link
Member

correctly extending baseproject and adding addons to the VSCode workspace

@roymacdonald
Copy link
Member Author

TL;DR

There is a lot of logic and parsing in addAddon that needs to be done in order to add get all the projects working. when it was virtual there was a lot of duplicated code in each of the different projects etc. So, the idea is that what gets extended by each project class are these . Then also, only when something different to what is implemented in these functions of baseProject, those should be overriden.

In the particular case of VScode project, I override addAddonBegin, which is there mostly because it was needed in VisualStudioProject to have it happening at the begining of the addAddon function. that is why it is named like that. In the case of VSCodeProject I choose to use it just because it has no clear intention as opposed to something like addAddonFramework, thus serves the purpose. As there is no addAddonToWorkspace or something like that, which feels unnecesary to add and bloats the baseProject class.

In any case, the idea is to explicitly not override addAddon function.

the addAddon functions are still public simply because these are being called from main.cpp, which is a mess I did not got all the way through but I have a good idea of what is going on (there is a lot of duplicated code, and a lot of code that never gets accessed.

@ofTheo @dimitre please merge this

@roymacdonald
Copy link
Member Author

@ofTheo can you please give me access to this repo ? so I can merge the cleanups, documentation, etc that is needed here without having to bother you

@ofTheo
Copy link
Member

ofTheo commented Nov 13, 2024

Done! :)

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.

2 participants