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

Fixing byte compilation issues #42

Merged
merged 6 commits into from Oct 12, 2021
Merged

Fixing byte compilation issues #42

merged 6 commits into from Oct 12, 2021

Conversation

phikal
Copy link
Contributor

@phikal phikal commented Sep 27, 2021

Hi,

I would like to add gnu-apl-mode to NonGNU ELPA, but before that is done, a few code issues should be fixed. The attached patches improve remove the deprecated cl library, and fix a number of issues raised by the byte compiler (see commit description).

If you decide to apply these changes, it would be necessary to bump the version attribute in the main file, so that ELPA recognizes a new release with these improvements.

@lokedhs
Copy link
Owner

lokedhs commented Sep 28, 2021

Thanks. This sounds like a good idea. At least until I can get the copyright assignments done so that it can be posted to ELPA.

I'll take a look at your proposed changes.

@phikal
Copy link
Contributor Author

phikal commented Sep 28, 2021

ELPA is always preferable, but I am not sure what the state if of the other contributors with significant contributions (@fourier, @doyougnu, @ecraven). It might be manageable, but if for whatever reason it wouldn't be, I think it would be preferable to just add the package to NonGNU ELPA, that doesn't require any CA.

@lokedhs
Copy link
Owner

lokedhs commented Sep 28, 2021

Yes, that's why I think it's a good idea. I have no idea when the copyright assignments will be done.

@fourier
Copy link
Contributor

fourier commented Sep 28, 2021

I have signed copyright papers and have code in ELPA if it helps.

@lokedhs
Copy link
Owner

lokedhs commented Sep 28, 2021

I'd prefer not to merge the changes as-is, since they change spaces to tabs. Can you recommit with only spaces?

The commits themselves looks fine to me, but I might have missed something due to the noise caused by the spaces being changed to tabs.

@phikal
Copy link
Contributor Author

phikal commented Sep 28, 2021

Sorry about that, should be fixed now. I also hope it is ok to add a .dir-locals.el to make sure that this doesn't happen again in the future.

Copy link
Owner

@lokedhs lokedhs left a comment

Choose a reason for hiding this comment

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

Thanks for your changes. I've added some notes.

gnu-apl-editor.el Show resolved Hide resolved
gnu-apl-follow.el Outdated Show resolved Hide resolved
gnu-apl-follow.el Show resolved Hide resolved
gnu-apl-interactive.el Show resolved Hide resolved
gnu-apl-network.el Show resolved Hide resolved
gnu-apl-osx-workaround.el Show resolved Hide resolved
gnu-apl-plot.el Show resolved Hide resolved
gnu-apl-spreadsheet.el Show resolved Hide resolved
@phikal
Copy link
Contributor Author

phikal commented Sep 28, 2021

If you are interested, I could also add the Makefile I was using to the project. It could be used to set up a CI (though I don't know how GitHub does that).

This primarily includes declaring various variables and functions,
replacing outdated conventions (usage of interactive commands,
outdated define-minor-mode usage) fixing a few docstrings and moving
functions around to improve visibility.
@lokedhs
Copy link
Owner

lokedhs commented Oct 7, 2021

If you are interested, I could also add the Makefile I was using to the project. It could be used to set up a CI (though I don't know how GitHub does that).

I don't mind at all. Integrating with the github builds is very easy.

@phikal
Copy link
Contributor Author

phikal commented Oct 7, 2021

Ok, as you see I have added the Makefile to this push request. I have no experience with GitHub Builds, so I hope you can take care of that.

@lokedhs lokedhs merged commit 97b4218 into lokedhs:master Oct 12, 2021
@phikal
Copy link
Contributor Author

phikal commented Oct 12, 2021

Thank you for merging the changes.

In case you are still interested: All that is left for NonGNU ELPA would be to update the version tag, as indicated in the first message.

@lokedhs
Copy link
Owner

lokedhs commented Oct 12, 2021

Thanks. 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.

None yet

3 participants