symmetryinvestments / imap-d

D library for IMAP (JMAP is a work-in-progress but the basics work)
13 stars 10 forks source link

Correct module name from `imap.imap` to `imap` #120

Closed nordlow closed 1 year ago

nordlow commented 1 year ago

FYI, @kinke.

codecov[bot] commented 1 year ago

Codecov Report

Merging #120 (a7958ee) into master (7e42dd7) will not change coverage. The diff coverage is 17.64%.

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   17.11%   17.11%           
=======================================
  Files          11       11           
  Lines        1268     1268           
=======================================
  Hits          217      217           
  Misses       1051     1051           
Impacted Files Coverage Δ
source/jmap/types.d 7.40% <ø> (ø)
source/symmetry/imap/auth.d 0.00% <0.00%> (ø)
source/symmetry/imap/request.d 0.00% <0.00%> (ø)
source/symmetry/imap/session.d 0.00% <0.00%> (ø)
source/symmetry/imap/set.d 0.00% <ø> (ø)
source/symmetry/imap/socket.d 0.00% <0.00%> (ø)
source/symmetry/imap/ssl.d 0.00% <0.00%> (ø)
source/symmetry/imap/system.d 0.00% <0.00%> (ø)
source/symmetry/imap/response.d 6.98% <6.98%> (ø)
source/symmetry/imap/namespace.d 93.54% <93.54%> (ø)
... and 1 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kinke commented 1 year ago

I think that'd be the nice variant. Another, non-breaking option would be to rename the file to imap/imap.d.

kinke commented 1 year ago

Or maybe combine the two variants - fixing up the module ID and adding a deprecated imap/imap.d module publicly importing the imap package.

Geod24 commented 1 year ago

I quite dislike top level modules because they tend to conflict easily with symbol. Why don't we have those in a namespace, e.g. symmetry.imap ?

adamdruppe commented 1 year ago

Yeah, single-name modules should be outright banned from the package repo since they're so prone to trouble.

symmetry.imap is a fine name.

and then the other subpackages should really be under it too.

kinke commented 1 year ago

Okay that makes totally sense to me. So to me it's a question whether to just rename the file to imap/imap.d for backwards compatibility, or going directly with the breaking symmetry prefix. - For context, this invalid module ID came up for a private Symmetry project, where import imap; (wrongly) worked, but later caused a codegen error with LDC wrt. the imap.imap ModuleInfo.

Geod24 commented 1 year ago

Use the symmetry prefix and provide a deprecated module that publicly import it. It will work for everyone and there is no downside to it. We don't even have to make a new major.

nordlow commented 1 year ago

Thanks for all the feedback.

In what file shall I use

module symmetry.imap;

and in what file shall I use the existing

module imap;

? On of them should be package.d, right. Should the other file be named imap.d and lie besides package.d?

Geod24 commented 1 year ago
git mv source/imap/ source/symmetry/
git mv source/symmetry/package.d source/symmetry/imap.d
sed -i -e 's/module imap/module symmetry.imap/g' source/symmetry/imap.d
echo >source/imap.d <<EOF
deprecated("Use module `symmetry.imap` instead")
module imap;
public import symmetry.imap;
EOF

Something along those lines.

nordlow commented 1 year ago

The changes I've made now gives

source/symmetry/imap.d(2,1): Error: module `symmetry.imap` from file source/symmetry/imap.d conflicts with package name imap

and I have no clue how to resolve this. Need help.

Am I missing a package.d file somewhere?

kinke commented 1 year ago

Use the symmetry prefix and provide a deprecated module that publicly import it. It will work for everyone and there is no downside to it. We don't even have to make a new major.

That's assuming everyone just imported the imap.imap package, not individual modules.

nordlow commented 1 year ago

Use the symmetry prefix and provide a deprecated module that publicly import it. It will work for everyone and there is no downside to it. We don't even have to make a new major.

That's assuming everyone just imported the imap.imap package, not individual modules.

Exactly. If not, we have to create a copies of those modules aswell. Shall we bother with that, @Geod24?

Geod24 commented 1 year ago

@kinke : Right, I was looking solely at the diff, not the other modules. Then yeah, we would need to create wrapper for all the other modules, which is a bit of noise, but a one-time cost. That is, of course, if everyone agrees having a top level symmetry package is a good idea (and it would keep in line with the previous kaleidic approach we used).

adamdruppe commented 1 year ago

and I have no clue how to resolve this. Need help.

this is a design flaw in the language. if you compile individually, it will allow imap.d to be module symmetry.imap but if you compile it together with the submodules, then the compiler arbitrarily and capriciously requires the file to be called package.d (unlike filenames in any other situation)

so this file:

source/symmetry/imap.d

must be renamed to package.d to silence this error.

I can't find the bug for this specifically but this random thing has caused a few bugs to be opened but this is by design so they won't fix it...

nordlow commented 1 year ago

this is a design flaw in the language. if you compile individually, it will allow imap.d to be module symmetry.imap but if you compile it together with the submodules, then the compiler arbitrarily and capriciously requires the file to be called package.d (unlike filenames in any other situation)

Thank you.

Have adjusted and dub tests pass locally now.

I'll create the remainder of the wrapper modules next.

Update. Ehh, CI fails at

https://github.com/symmetryinvestments/imap-d/actions/runs/3265512913/jobs/5367843519#step:6:119

as

source/imap.d(3,15): Error: unable to read module `imap`
source/imap.d(3,15):        Expected 'symmetry/imap.d' or 'symmetry/imap/package.d' in one of the following import paths:

. Update: Ahh, I see now. Going green now.

nordlow commented 1 year ago

Should be ready now. Unfortunately git doesn't figure out a smart final diff for this one. But I believe this is ok if the review is split up into

f262ac9 and https://github.com/symmetryinvestments/imap-d/compare/f262ac91d0501fb5cf73fc948db7be5183b1d851..a7958eece6c2745e03bf7167f84bb3e5c547446d

nordlow commented 1 year ago

Ping.