Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix export to only export symbols of current package. #7

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

Ambrevar
Copy link
Contributor

Previously, defclass-star* would fail when overriding a slot of a superclass.
Example recipe:

    (setf hu.dwim.defclass-star::*export-slot-names-p* t)
    (hu.dwim.defclass-star:defclass* foo ()
        ((foo-name :initform "foo name")))

    (defpackage foo-package
      (:use :cl))
    (in-package foo-package)

    (hu.dwim.defclass-star:defclass* bar (cl-user:foo)
        ((cl-user:foo-name :initform "bar name")))

Previously, defclass-star* would fail when overriding a slot of a superclass.
Example recipe:

    (setf hu.dwim.defclass-star::*export-slot-names-p* t)
    (hu.dwim.defclass-star:defclass* foo ()
        ((foo-name :initform "foo name")))

    (defpackage foo-package
      (:use :cl))
    (in-package foo-package)

    (hu.dwim.defclass-star:defclass* bar (cl-user:foo)
        ((cl-user:foo-name :initform "bar name")))
@attila-lendvai attila-lendvai merged commit 39d458f into hu-dwim:main Jan 29, 2021
@attila-lendvai
Copy link
Contributor

this broke our codebase in a non-trivial way.

@Ambrevar are you sure that this is the right fix for whatever issue you experienced?

@Ambrevar
Copy link
Contributor Author

Ambrevar commented Dec 1, 2021

It's been working for Nyxt ever since.
At first glance I can't see what's wrong with the patch. Could you elaborate with what went wrong on your end?

@attila-lendvai
Copy link
Contributor

AFAIU, it fails to export the NAME-OF symbol from hu.dwim.walker, due to which the originally single generic method got split into two, the other one being a now private/isolated NAME-OF in hu.dwim.meta-model.

or to put it another way: our codebase expects that symbols get exported, regardless of what is their home package.

i have rewound the stable tag, and this should fix quicklisp for now, but this needs to be addressed eventually.

@attila-lendvai
Copy link
Contributor

i also opened an issue not to forget this.

@attila-lendvai
Copy link
Contributor

@Ambrevar i just understood that rewinding defclass-star in ql breaks nyxt (until now i didn't know what it is, or that it's part of ql).

how about reverting this single commit, at least for the time being? does nyxt depend on it?

@Ambrevar
Copy link
Contributor Author

Ambrevar commented Dec 1, 2021 via email

attila-lendvai added a commit that referenced this pull request Dec 1, 2021
It breaks the hu.dwim universe.

More info: #7
@attila-lendvai
Copy link
Contributor

i've reverted it, and updated the stable tag. ql should be all green.

@attila-lendvai
Copy link
Contributor

FTR, i do not consider the current behavior a bug.

the CL symbol exporting operation does not care whether the home package of the symbol being exported is the same package it is being exported from. i do not see any reason why the default behavior should not be aligned with the semantics of CL's own export operation.

and introducing a non-trivial bug into our codebase is only a wakeup call, not the reason for this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants