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

Rewrote CI to Pascal #16

Merged
merged 7 commits into from
Jan 18, 2025
Merged

Rewrote CI to Pascal #16

merged 7 commits into from
Jan 18, 2025

Conversation

theavege
Copy link
Collaborator

What do you think about this CI?

@circular17
Copy link
Owner

Wow this is great stuff, everything done in Pascal 🥇

Would the Pascal scripts allow use of units? Because if the code can be refactored, you could have one helper unit that could be used in many repositories, and keep the main Pascal makefile short and easy to read.

By the way, I wonder why it is needed to have the UniqueInstance component.

Warm regards

@theavege
Copy link
Collaborator Author

By the way, I wonder why it is needed to have the UniqueInstance component.

Removed.

Would the Pascal scripts allow use of units?

Yes

@circular17
Copy link
Owner

Thanks.

I suppose in procedure AddPackage, you would like to use the Path variable as parameter when calling FindAllFiles :

    List := FindAllFiles(Path, '*.lpk', True);

@circular17
Copy link
Owner

Looks good to me and CI passes.

I would suggest to rename the Code member of Output to Success as it is a boolean that is nonzero when successful.

I can merge the pull request if you like. Or I can wait if you would like to continue refactoring. What would you prefer?

@theavege
Copy link
Collaborator Author

I suppose in procedure AddPackage, you would like to use the Path variable as parameter when calling FindAllFiles :

Thank you. I fixed.

@circular17
Copy link
Owner

I would suggest to rename the Code member of Output to Success as it is a boolean that is nonzero when successful.

What are your thoughts on this?

@theavege
Copy link
Collaborator Author

What are your thoughts on this?

@circular17 I have done

@theavege
Copy link
Collaborator Author

@circular17 Could you please approve this PR?

@theavege theavege merged commit a00ec2d into circular17:main Jan 18, 2025
1 check passed
@theavege theavege deleted the upd/ci branch January 18, 2025 20:20
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