Skip to content

Revert the style2str change in b547ead #8721

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

Open
phadej opened this issue Feb 1, 2023 · 3 comments
Open

Revert the style2str change in b547ead #8721

phadej opened this issue Feb 1, 2023 · 3 comments

Comments

@phadej
Copy link
Collaborator

phadej commented Feb 1, 2023

The commit b547ead introduced change to style2str. I think it's wrong. Or at least I'd want to see a lot better justification for it then just changing inside unrelated patch.

In particular, there are no tests. As a maintainer of cabal-plan I want to understand the change, but I don't. So please revert it, and do it properly if there is justification.

@jneira
Copy link
Member

jneira commented Feb 1, 2023

Just in case, from the commit message:

Additionally, adjust the ‘style’ attribute in plan.json so that globally
installed packages are designated as global even if they are local to the
project. (Without this adjustment to ‘style2str’, the T5782Diamond test fails,
because it looks up ‘dist-dirs’ in plan.json, where ‘dist-dirs’ is absent from
the JSON.)

But there is no specific tests about that change (but it did not broke any prior test so the previous behaviour was not tested either?)

@Mikolaj
Copy link
Member

Mikolaj commented Feb 1, 2023

@jneira: yes, it seems it wasn't tested. Which just proves you right when you go around pestering everybody to add their tests to their PRs. :D

@phadej
Copy link
Collaborator Author

phadej commented Feb 1, 2023

But there is no specific tests about that change (but it did not broke any prior test so the previous behaviour was not tested either?)

There kind of was. The beef of b547ead broke T5782Diamond test. So instead of thinking that changing what local package means is maybe not the right way, the author made another somewhat unrelated change elsewhere.

It isn't a direct test about what plan.jsons local (note: this local is different from local in LocalTarball, local tarballs maybe global or inplace - but they are not local - confusing isn't it), inplace and global means, but I explain my best understanding in #8722 (comment). The b547ead introduced the fourth combination in ElabConfiguredPackage flags , which I think shouldn't ever happen, as it doesn't make sense to me. OTOH, it's not prevented by the types, so it probably happen occasionally.

I'd go and try to change the two booleans into a type with only three values (provide selectors with old names for compat), and see what other changes need to be done. But it's not a few hour experiment, so I won't have time to do it.

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

No branches or pull requests

3 participants