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

support src/ directories #115

Open
glyph opened this Issue Jun 25, 2017 · 51 comments

Comments

Projects
None yet
@glyph

glyph commented Jun 25, 2017

A src/ directory is a best practice these days (outlined here: https://hynek.me/articles/testing-packaging/#src ); it ensures you've made your package importable on purpose, and helps make sure you're not just picking up the one in cwd just because you happen to have a shell open in that directory.

I think all flit would need to do is sys.path.append here in some cases.

@takluyver

This comment has been minimized.

Owner

takluyver commented Jun 25, 2017

I'm inclined not to, because:

  1. Part of the reason I wrote flit was to get away this sort of variation which doesn't have an obvious reason: I want there to be one obvious way to lay out a repository containing a Python package, not two different ways for people to argue about.
  2. I also tried to design flit to minimise the kind of issues that article talks about avoiding - differences between the package in the source tree and the installed package, e.g. because you forgot to add a file to package_data. I want the installed package to automatically be as similar as possible to the installed package.
@glyph

This comment has been minimized.

glyph commented Jun 25, 2017

Part of the reason I wrote flit was to get away this sort of variation which doesn't have an obvious reason: I want there to be one obvious way to lay out a repository containing a Python package, not two different ways for people to argue about.

I didn't say that you had to keep supporting non-src layouts :). Having there be only one way to do it is valuable; it's just that the "one way" which seems to be increasing in popularity is src, not ..

@glyph

This comment has been minimized.

glyph commented Jun 25, 2017

I also tried to design flit to minimise the kind of issues that article talks about avoiding - differences between the package in the source tree and the installed package, e.g. because you forgot to add a file to package_data. I want the installed package to automatically be as similar as possible to the installed package.

I recognize that, but as long as flit install exists, there remains the unfortunate possibility for version confusion.

@glyph

This comment has been minimized.

glyph commented Jun 25, 2017

Maybe the thing to do here is that flit.ini should just go in src/, and things like docs/, etc, can live outside of it?

@takluyver

This comment has been minimized.

Owner

takluyver commented Jun 25, 2017

Dropping support for non-src layouts would be a nasty breaking change, though. I'd need a pretty compelling reason to go through all that, and the thing with tests doesn't seem like that big a deal (or it seems like something that could better be mitigated by testing tools). The vast majority of packages I see use the . layout, and I'm happy with it for my own use.

Maybe the thing to do here is that flit.ini should just go in src/, and things like docs/, etc, can live outside of it?

I wouldn't recommend that in general: the sdist will only include files from the directory where flit.ini (later pyproject.toml) lives.

@takluyver

This comment has been minimized.

Owner

takluyver commented Jun 25, 2017

I meant to add: but if you're willing to live with that limitation, putting flit.ini inside src/ should work to make a package that can be installed correctly.

@glyph

This comment has been minimized.

glyph commented Jul 8, 2017

The one problem with that limitation is that, in the halcyon future where pip install out of a Github URL might actually work with flit (i.e. when we have a pyproject.toml), you'd have to remember that super weird syntax to install git URLs that aren't at the root.

It would also mean that you'd have to do flit install ./src instead of flit install . yeah?

@glyph

This comment has been minimized.

glyph commented Jul 8, 2017

The major reason that this convention is important is to correctly reflect things like source code coverage when the software in question is run under test. So another option here would be to fix that upstream in tox.

@sanjioh

This comment has been minimized.

sanjioh commented Aug 2, 2017

+1
Being able to set a custom base dir into which make flit look for packages was the first option I searched the docs for. FWIW, I don't think that would make the usage less obvious (the default can be the current dir as it is now). Having import parity during tests is priceless :)

@ionelmc

This comment has been minimized.

ionelmc commented Aug 23, 2017

So the argument here against the src-layout is that flit doesn't have the problems of setup.py (the typical blunders regarding setup() arguments) - so then why bother with all the cruft if what gets installed is the same stuff you have in the project root right?

This sounds right, however it is not: because flit is not self-configuring (you still need to list what gets installed, the [metadata] module setting) you're still prone to goofing up the published wheel. Plus flit could always have a bug ... even if it's way smaller than the unholy duo (you know what I'm referring to, for sure 😁).

Using a src-layout would solve 3 long standing issues in flit:

  • Lack of self configuration. If all the code is in src then it's easy, you collect everything from there and you're done. No configuration.
  • Lack of support for multi-module/package distributions. Currently it's impossible to distribute multiple modules/packages with flit.
  • Lack of "proof of correctness". Currently it's very hard to prove your distribution is correct. Test tools have to change to a tmpdir to be absolutely sure the installed package is imported.

The workaround that @takluyver proposes (move flit.ini to src and maybe change flit to also lookup configuration in src?) only solves that last issue.

@glyph

This comment has been minimized.

glyph commented Aug 24, 2017

Thanks @ionelmc for articulating these concerns much more precisely than I did :)

@takluyver

This comment has been minimized.

Owner

takluyver commented Aug 24, 2017

because flit is not self-configuring (you still need to list what gets installed, the [metadata] module setting) you're still prone to goofing up the published wheel.

You 'list' one name, and if you specify a module that's not there, it will fail when you build the wheel. I accept that it's possible to get it wrong in such a way that you build an incorrect wheel, but in practice I think it's pretty unlikely.

Lack of support for multi-module/package distributions. Currently it's impossible to distribute multiple modules/packages with flit.

This is by design. I'm not convinced there's any real value in allowing multiple top level modules in one distribution - I suspect it's a leftover from before nested packages were possible - so I built flit with the idea that one distribution installs one package.

@ionelmc

This comment has been minimized.

ionelmc commented Aug 24, 2017

The main usecase (that fixing those issues would support) is:

I have a bunch of files here, publish them for me. I don't want to deal with configuration. Maybe later.

So basically a mode where stuff in src/ is published, and metadata is filled from whatever is there in the checkout (homepage=repo url, dist name=package name if there's a single package). A flit.ini is created, and users can customize it later.

@takluyver

This comment has been minimized.

Owner

takluyver commented Aug 24, 2017

flit init already does something like this without needing a src directory. It would also be possible to make a similar tool that does more - e.g. getting the repo URL from git - but I think that should be made separately from flit.

@ionelmc

This comment has been minimized.

ionelmc commented Aug 30, 2017

Now that I think about it more, it's right that flit would not support the mostly fringe usecases (like multi-module/package distributions). There's an impossibly long list of things setuptools/distutils can do.

However, problems will appear when projects want to "grow out of flit". At some point you need a C extension or need to use cython or other weird usecase. Then you need to switch to setuptools and then only pain awaits if you have the non-src layout. This is why I think it's important to steer users towards a better layout.

I guess the 'put flit.ini in src' compromise could work ...

@takluyver

This comment has been minimized.

Owner

takluyver commented Aug 30, 2017

At some point you need a C extension or need to use cython or other weird usecase. Then you need to switch to setuptools

I think there are a whole lot of interesting Python libraries which can be made without a C extension. And there are other options besides setuptools, such as enscons. One thing we've discussed (on #119) is having a tool which can convert flit metadata to a skeleton enscons build. Looking at the example packages, enscons doesn't use src/ layouts by default either - though I expect it's flexible enough that you can if you want to.

@dholth

This comment has been minimized.

dholth commented Aug 30, 2017

The src layout is the kind of thing that seems like a good idea, but I almost never use it in practice. Only for work projects.
I think it's going to be really powerful to just switch build systems when you need another feature. Once your project becomes more complicated, it will not seem like too much extra work to configure that more complex build system.

@pfmoore

This comment has been minimized.

pfmoore commented Sep 14, 2017

I'm in the process of looking at moving to flit for my projects. However, I'm also in the process of switching my standard project layout to have a src directory. This is because I've had repeated issues in the past with locally run scripts picking up the wrong version (the working version, rather than the installed copy). This is particularly annoying when I'm working on the "next version" of something I use routinely. Not being able to have my current directory be the project directory and the working code in a src subdirectory (with tests in a tests directory, invoke tasks in a tasks directory, documentation in a docs directory, etc) is going to be a step backwards for me. If flit doesn't support the src directory convention, I'm going to have to choose whether to drop that or to drop flit.

I know tox can have problems with sources in the top-level directory, too, although I don't recall the details and I'd have to do some tests to come up with a specific scenario. However, I'm not sure if tox works with flit yet. Does it? If not, then that's a showstopper for me, and I'll have to stick with setuptools for now (and hence my views on the src layout are less relevant, I guess).

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 14, 2017

Are you making a case that the src/ layout should be an available option, or that it should be what everyone does? I'm somewhat more sympathetic to making it the 'one obvious way to do it' (with an appropriate transition). But at the moment the 'one obvious way' is the other way, and I'm not convinced that the benefits people have described so far outweigh the pain of switching.

However, I'm not sure if tox works with flit yet. Does it?

Not as far as I know. I don't really use tox myself, but I would imagine that it won't support flit until it implements PEP 517 - which still isn't accepted yet.

@pfmoore

This comment has been minimized.

pfmoore commented Sep 14, 2017

I'm saying that flit should support the src option. If you're arguing that flit only support one layout, then I guess I'm saying that should be the src layout - but only because of the design choice to not allow multiple layouts. I don't honestly see why you are so strongly against a simple source-dir=xxx option, with a default of the top-level directory, so I can't really debate that.

While I see your point about "one obvious way", I genuinely don't think there's consensus on this issue at the moment. If you want flit to be opinionated, and mandate a particular layout, then that's fine (although you may lose some users who have a different opinion) but that's a subtly different position than only supporting the "obvious" approach.

As far as tox is concerned, I'll try it out. But if flit won't work with the "obvious" (😄) standard workflow for Python projects of pytest/unittest plus tox, plus github with travis CI, etc, until the various pieces support PEP 517, then maybe it's a bit early for this debate.

@pfmoore

This comment has been minimized.

pfmoore commented Sep 14, 2017

... on tox support (as a follow-up) - it appears that tox hard-codes setup.py sdist as the command to generate a sdist, at the moment. So no, tox won't work with flit (although there is an issue on the tox tracker about PEP 517/518 support, so it's a medium-term goal).

For now, though, short of writing some sort of setup.py wrapper for flit, I guess standard tool support for flit is still a bit of a way off.

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 14, 2017

I'm not strongly against that option in particular. But every option increases complexity, and I bet it's not as simple as it appears at a casual glance. (E.g. is specifying a parent directory allowed? Are there security considerations? Will users try to specify a build directory as 'source' to make a hackish way to compile stuff before running flit? What about if they specify a path which is in another VCS repo - how does that affect sdist builds?)

I also try to consider how this appears to people who are getting comfortable with Python code but are new to packaging. One of the complaints about setup.py packaging is that there's a surfeit of confusing options, and the option for different layouts of your source tree seems like a prime example. Decisions are work that humans have to do, so it's ultimately better to say 'this is how you lay out code to make a package' than to push that decision onto users.

Finally, it's valuable to go to a Github repo and know immediately where things are. If you make it optional, you increase the chance that you have to stop and work out what the author has chosen for this package.

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 14, 2017

That's pretty much what I'd expected with tox. If using tox is important to you, I'd probably hold off on using flit for now. It's entirely possible to use tools like py.test on flit packages (I do so on flit itself), but probably not any tool that tries to deal with building/installing the package before testing.

@Carreau

This comment has been minimized.

Collaborator

Carreau commented Sep 14, 2017

I'm in the process of looking at moving to flit for my projects. However, I'm also in the process of switching my standard project layout to have a src directory. This is because I've had repeated issues in the past with locally run scripts picking up the wrong version (the working version, rather than the installed copy)

Why I see why, and I use to think that src was better, I'm now annoyed at project using src or lib. (Though you have more experience than me), and some project that use src or any other directory not named like the package confuse newcomers quite a bit (IMHO).

I'm with thomas that I'd like to have one – and only one – obvious way, and for flit to be opinionated. If this implies moving all to /src/ I'm not going to be the happiest, but I'll live with it and get used to it. I would prefer this to grow too many configurations option.

If would be nice to agree with the packaging guide, regardless of wether none or both need to be updated, and once pep 517 is accepted, if this could be fold in a flit upgrade command that takes care of everything that would be awesome.

@dholth

This comment has been minimized.

dholth commented Sep 14, 2017

@pfmoore

This comment has been minimized.

pfmoore commented Sep 14, 2017

If would be nice to agree with the packaging guide, regardless of wether none or both need to be updated,

Definitely. However, the packaging guide has traditionally been fairly neutral over how people structure their projects. So it may not ever make a specific recommendation (although I do expect it to discuss both options at some point - there's an issue open against it to mention the src approach).

@takluyver

This comment has been minimized.

Owner

takluyver commented Sep 14, 2017

I think neutrality is the wrong approach for the guide to take. I think it's a more useful guide if it recommends a standard file layout for Python libraries to use.

We've had a similar debate over the installation instructions for Jupyter. In both cases, I think that 'neutrality' is a way to satisfy a group of writers who disagree over the best way to do something, at the cost of being less helpful for the reader.

Daniel:

Or one could just make a fork of flit that's exactly the same except it only uses src directories.

One certainly could, but then you're adding another choice about what tool to use. If we fork every time we disagree, the packaging ecosystem will soon be very confusing and offputting. It also doesn't do anything for my argument about 'obviousness' - I want as many packages as possible to use the same layout, so that it's easy for people to look at and contribute to a wide range of packages.

I think this discussion is bigger than flit - it's about whether we recommend src/ layout as the norm. Can I suggest that we take it up on distutils-sig? But please, after PEP 517 is provisionally accepted - I don't want to start any discussion that will distract from finishing that.

@pfmoore

This comment has been minimized.

pfmoore commented Sep 14, 2017

I think neutrality is the wrong approach for the guide to take. I think it's a more useful guide if it recommends a standard file layout for Python libraries to use.

Personally, I agree. My biggest problem is actually that I don't personally have the experience to have firm opinions.

I think this discussion is bigger than flit - it's about whether we recommend src/ layout as the norm. Can I suggest that we take it up on distutils-sig?

I think you're right. I suspect that any discussion on distutils-sig will end up coming to no conclusions (it's not traditionally been a great forum when it comes to easily achieving consensus 😉) but that's certainly the right place to discuss this.

But please, after PEP 517 is provisionally accepted - I don't want to start any discussion that will distract from finishing that.

Absolutely!

@dholth

This comment has been minimized.

dholth commented Sep 14, 2017

The packaging guide has always been problematic to me. Apart from the regrettable but understandable decision to recommend setup.py, it should be much shorter and much more opinionated, and it should recommend the flatter layout since those who need a src directory no longer need the guide.

@ofek

This comment has been minimized.

ofek commented Sep 25, 2017

For those using src/ layouts due to testing ambiguity.. have you considered installing in editable mode instead i.e. pip install -e? That solves the issue completely.

@pfmoore

This comment has been minimized.

pfmoore commented Sep 25, 2017

Um... I don't think flit supports editable mode? There's the --symlink option to flit install but that doesn't work on Windows, I assume (due to symlinks not working without admin rights). Also, there is no testing ambiguity with the src layout - the ambiguity is using the non-src layout.

@ofek

This comment has been minimized.

ofek commented Sep 25, 2017

@pfmoore Sorry, I meant just generally as I'm unfamiliar with flit :)

Yes I am saying if you are using src/ just to avoid testing ambiguity then doing what I mentioned is an alternative.

https://github.com/ofek/hatch tests projects this way by default as do others

@hynek

This comment has been minimized.

hynek commented Sep 26, 2017

pip install -e . is basically the reason why I use src. The ambiguity arises from not knowing for sure whether my imports use the installed version (if the module was installed properly!) and not some random stuff laying around in my current directory.

@pfmoore

This comment has been minimized.

pfmoore commented Sep 26, 2017

There's always ambiguity if your project is in your cwd - Python puts the cwd on sys.path, but not all testing tools do. The src convention makes it so that you're less likely to have your project in your cwd, so reducing the ambiguity, and editable installs make it so that you don't need the project in your cwd in order to do adhoc tests/experiments etc.

Honestly, though, I don't have a problem with people preferring not to use src, if the simpler structure suits them and they don't find the problems affect them. I do have a bit of a problem recommending (or choosing) flit for new projects if I'm going to have to switch to a different tool if and when the project grows to the point where a src structure becomes beneficial (in my experience, that's a relatively common situation).

I'd like to think it would be simple for flit to have a package-directory=src entry in flit.ini to direct it where to look for the package code (defaulting to the project root, for backward compatibility). But it's @takluyver's call as to whether that's actually as simple as I think 😉

@Juanlu001

This comment has been minimized.

Juanlu001 commented Oct 26, 2017

I think I am not the only one excited about pip 10, PEP 517 and PEP 518 and considering flit as a replacement for setup.py. But I share the pain of those who have switched to an src layout and do not intend to go back, so if flit is going to be opinionated and disavow the use of src layouts, then I might as well stay with plain old setuptools. @ionelmc packaging slides and articles from 2015 are still my reference and I also echo the things that @pfmoore has already said, so I have nothing meaningful to add in support of src layouts. I also do not claim that they are TheRightWay™, but would love to have tools that support them anyway. My two cents.

@pradyunsg

This comment has been minimized.

pradyunsg commented May 13, 2018

I'll be happy to help out by implementing this.

@takluyver

This comment has been minimized.

Owner

takluyver commented May 13, 2018

It's more a design question than something to implement.

I don't have a strong preference between src/ layout and the layout flit currently uses, but I do have a strong preference that there should be one way to do it. This doesn't feel like something where the choice itself is important, and having one way makes it easier for new contributors to quickly grok any project they come across.

If people feel strongly about this, I'd suggest we start a thread on the distutils-sig mailing list about whether it's worth recommending the src/ layout in general. If there's a consensus that the advantages of that outweigh the disadvantages + transition costs, then we can figure out a transition plan for flit.

@pradyunsg

This comment has been minimized.

pradyunsg commented May 13, 2018

It's more a design question than something to implement.

Agreed. :)

I'd suggest we start a thread on the distutils-sig mailing list

I guess pypa/python-packaging-user-guide#320 first would be better way to go about this.

@TiemenSch

This comment has been minimized.

TiemenSch commented Oct 15, 2018

As there are tons of reasons to have your package code in any other folder than where your .toml resides, I would say a modules_root option somewhere should be considered essential to flit's interface.

I think one should be able to both point to a .toml in an arbitrary location as well as a modules_root. Flit works wonderfully to publish your Python packages with a single .toml file, but right now it is forcing potential users through a hoop that may be impossible for them to jump through for a multitude of reasons (of which src/ is just one).

With the current stance, flit positions itself as a tool as well as a packaging authority. I don't mind for a tool being opinionated, but parameterization with opinionated defaults is a better way to go IMHO. And then just let PyPA handle the authority thing as soon as they reach a consensus :).

@dholth

This comment has been minimized.

dholth commented Oct 16, 2018

No one uses src/ directories.

@uranusjr

This comment has been minimized.

uranusjr commented Oct 17, 2018

Except, of course, two PyPA projects actually do (I checked).

@Juanlu001

This comment has been minimized.

Juanlu001 commented Oct 17, 2018

There are many people subscribed to this conversation, so please let's keep it trolling-free.

@TiemenSch

This comment has been minimized.

TiemenSch commented Oct 17, 2018

Turns out the Module() class already had a directory parameter, which made an initial implementation easier than I thought.

I went ahead and implemented a module_root key to the metadata section in this branch:
https://github.com/TiemenSch/flit/tree/module_root

I tested it with moving the flit dir into src/ and ran python -m src.flit -s just fine on my machine to install the development version.

Passes all current tests on my Windows machine as well, but it is in need of its own tests!

The name of the key is just a placeholder. I chose for a metadata key such that successful installs don't depend on which arguments you use.

@takluyver

This comment has been minimized.

Owner

takluyver commented Oct 17, 2018

Just to clarify my position:

  1. PyPA is having a discussion on what layout to recommend at pypa/python-packaging-user-guide#320 . I think this is gradually heading towards a decision to recommend the src/ layout, but until that gets there, I don't plan to change anything in Flit (even if someone else does the coding, sorry ;-).
  2. Flit is built around the idea that packaging shouldn't involve lots of choices. So if PyPA does recommend the src/ layout, I'm inclined to make it mandatory for Flit, not a configurable option. There will be a transition period to give people time to change their layout, but eventually I expect that transition period to end.
@TiemenSch

This comment has been minimized.

TiemenSch commented Oct 17, 2018

I think it would especially make sense to support both ways as long as there isn't a clear decision over at PyPA. Also, I deem it basic functionality. But hey, opinions differ sometimes :).

@pradyunsg

This comment has been minimized.

pradyunsg commented Nov 25, 2018

@takluyver TBH, as things stand, IMO it's possible that PyPA won't recommend the src/ (or non-src/) layout and instead document the trade-offs of both the layouts in a guide. It's not uncommon for projects to start with non-src and change to the src layout when they get bigger.

What are your thoughts on supporting both layouts in flit, given this state?

@takluyver

This comment has been minimized.

Owner

takluyver commented Nov 25, 2018

My thoughts are basically still the same: I think for this kind of thing it's a big benefit to have 'one obvious way to do it'. If PyPA makes a recommendation, that's the one obvious way, and Flit will implement it. In the absence of a clear recommendation, the status quo wins by default, because changing things is always tricky. I'm not interested in supporting both except as a transition period.

Documenting trade-offs is a cop out. A guide to packaging should - at least as a starting point - lead people through one clear way of doing it, not ask them to make a decision that the experts can't agree on. I know I'm implicitly demanding that someone else make this decision for me - I'll go and review the thread and see if I can help move it forwards.

@pfmoore

This comment has been minimized.

pfmoore commented Nov 25, 2018

Documenting trade-offs is a cop out.

If it were just "documenting trade-offs", then I think you have a point. But my view is that for small, simple projects, the non-src layout is the best approach. But once a project grows, it should switch (i.e., IMO the debate is over when to switch, not over which to use). Recommending different answers for different needs isn't a cop-out, but an acceptance that not all situations are the same. So what I'd like to do is to make the PyPA recommendation "start with the non-src layout, but switch to a src layout once you have a non-trivial test suite and deployment process". (There would need to be work to make that more precise, and that's where "explaining the trade-offs" comes in).

My concern is that, by targeting simpler projects, and insisting on only supporting one layout, flit derails that recommendation - the cost of switching to the src layout should be pretty minimal, but flit's position adds "... and change your build system", which is a much more significant cost - probably enough to make projects stick with the non-src layout far beyond the point where the PyPA recommendation would be to switch.

Obviously, the flit project has to decide what's best for it. But passing the buck to the PyPA to pick a layout recommendation, while still refusing to support a 2-option recommendation, results in the impasse we currently find ourselves in.

PS I think the reasoning over why have a 2-level recommendation has already been made (implicitly) in the debate over at pypa/python-packaging-user-guide#320, so I don't want to rehash that here, but if you want me to expand, feel free to ask over there.

@takluyver

This comment has been minimized.

Owner

takluyver commented Nov 25, 2018

So I'd consider whatever is in the packaging tutorial to be PyPA's recommendation, which is currently for a non-src layout.

From my perspective, the benefits of having 'one obvious way to do it' outweigh the benefits of having different recommendations for different cases.

@pfmoore

This comment has been minimized.

pfmoore commented Nov 25, 2018

So if that tutorial said "start with a non-src layout and change to a src layout as your project becomes bigger, what would flit do?

I'm asking because I'm becoming more convinced that this is what I'd like to see happen, and I'm unclear as to what flit would do in that case. At the moment, that tutorial recommends setuptools, but I want to try to get an understanding of whether we could change that to flit at some stage in the future. My current feeling is that if flit doesn't support the src layout, we can't really do that (because it doesn't make sense to recommend changing layout as a project grows, and yet recommend a build tool that won't support that change).

@takluyver

This comment has been minimized.

Owner

takluyver commented Nov 26, 2018

I'm not sure. This kind of inconsistency is what I was trying to get away from with Flit: I wanted packaging that was more "do this and this, and you're done", and less "here are three different options, and a list of blog posts arguing about which is best". Of course, creating Flit added another option, but...

I might well decide that a split recommendation just means it hasn't really been decided yet, and leave Flit doing what I'm used to. If it works for me, I don't feel much obligation to make it work for every use case out there. I might give in and decide that supporting both is a necessary compromise. I might ask PyPA to adopt Flit (as has been discussed in the past), and let someone else implement what I'm not interested in writing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment