themoken / canto-next

The next generation Canto RSS daemon
GNU General Public License v2.0
158 stars 10 forks source link

Mind if I add XDG? #20

Closed ghost closed 9 years ago

ghost commented 9 years ago

I want to add XDG folder support to Canto. Mind if I submit the patch upstream?

themoken commented 9 years ago

I wouldn't mind. In fact, you can make it default, as long as it will still use ~/.canto-ng if it exists. Make a pull request and I'll merge it if it looks good.

ghost commented 9 years ago

Sorry for the delay. End of semester is pretty busy. I'll be able to do this come the 10th.

ghost commented 9 years ago

I'm just going to close this and wish you luck... The config location is set in the protocol module of the server and apparently I can't find it in the client. :-1: I don't want to dishearten you but this code is really not server grade. Both projects need refactoring and the protocol needs to be addressed for proper boundary separation.

Aim for simpler code and use object oriented code where appropriate. Good OOP coding drastically reduces conditional logic use by utilizing polymorphic object handles with inherited behaviours. This code isn't really OO except for the use of classes. I'm truly not one to preach about good coding but it was extremely hard locating anything I was looking for.

Lastly, run a lint like pyflakes, flake8, or prospector over your code. My editor lit up like a Christmas tree when I saved some changes. Many PEP8 mandates are a little weird and can catch out even experienced coders.

Hope this doesn't get you down. Instead, I hope it helps you see some improvements that can be made to really make Canto spectacular.

themoken commented 9 years ago

This is a hugely condescending post for someone that couldn't figure out that the directory location logic in protocol.py is shared between the daemon and all clients via inheritance. Especially ironic considering you think there should be more OOP and polymorphism and not less.

XDG support would amount to maybe 10 lines in a single location (that you found but failed to understand) and it would work seamlessly with everything in the ecosystem (daemon, remote, clients), and yet somehow my code needs refactoring? You can't even grasp my code and you're criticizing it as "not server grade"? What a joke.

No hard feelings, but the next time you can't figure out a 10 line patch to what is arguably the most basic part of a project, perhaps you should try a little harder instead of blaming the code. Worst case, ask a person that actually knows how it works before you embarrass yourself being critical of code you don't understand.

langner commented 9 years ago

Wow. Make sure to fix all those stylistic errors :P

ghost commented 9 years ago

@themoken, I thought you would take it this way. Again, I'm not saying I'm in any way better at coding and I'm not trying to make this personal. My suggestions were, only that. I was able to locate the code and like I said, creating a patch is simple. I've included it below if you still want it:

canto-next/protocol.py:83

if os.path.isdir(os.path.expanduser(os.path.join(
                 os.getenv.get('XDG_CONFIG_HOME'), "canto-ng/")):
    self.conf_dir = os.path.expanduser('~/.canto-ng/')
else:
    self.conf_dir = os.path.expanduser(os.path.join(
                    os.getenv.get('XDG_CONFIG_HOME'), "canto-ng/"))

I don't think I'm holier than thou at coding. I just found Conto to be oddly structured with logic often spread out in multiple places. For example, the first thing that struck me was that arguments weren't all parsed in the same location. I understand you code base as well as a couple hours' glance would provide. I claim Canto is 'not server grade' for two reasons. The first was when I found canto-next/protocol.py:211 cmd, args = eval(repr(json.loads(message)), {}, {}) which as far as I could tell, leads to remote code execution. I haven't tested this but the fact that eval is called on string objects worries me. The second is that using the client requires the server to also be installed.

I came to Canto from my own feedreader based RSS reader hoping to contribute what I could to create a better unified reader with bigger and better possibilities for it. I'm just not feeling the code behind the project and have chosen another instead. I wouldn't have said anything but I myself appreciate all the feedback I can get on my work. I've offended you in the process and I'm sorry; wasn't my intent.

Kind regards and good luck going forward - Keller

themoken commented 9 years ago

If your feedback had included specific examples in your last post, I would've been more than happy to debate the merits of your arguments, but you have to see how your presentation was less informative and more inflammatory. There's a reason that you couched most of it trying not to dishearten me.

So, as for the eval(repr(x)) pattern, that's an idiomatic way to make a fresh (referenceless) python copy of an item. In this case, it's operating on an object that's already been created by JSON, so if there's remote code execution it's because of a vulnerability in the JSON module, not here. You can't for example, embed python into JSON or C calls, etc. - whatever comes out of that loads() call is going to be a simple data structure.

With regards to the clients requiring the daemon... Canto's only supported usecase is with the client and daemon on the same machine communicating through a local unix socket. It has the ability to use an internet socket, but it's inherently insecure without SSH (as noted in the --help for any of these programs). This is also why the arg parsing is split into "common to all" arguments in protocol.py that deal with locating the proper (usually local) socket, and specific args for the daemon and clients.

I guess what you're saying with "not server grade" is that it's not enterprise like Apache or something? Which, okay, then I guess I have to agree. It's a single-user local daemon like MOC. The only reasons that it's even a server is to keep fetching in the background without a crontab entry, to circumvent the GIL, and to abstract most of the pure feed logic so alternate front ends could be written - not so that it could replace something like TT-RSS that aims to be a multi-user, internet facing platform.

Anyway, if you want to provide feedback to a project in the future, please stick to code examples and specifics like these instead of vague generalizations and I think you'll find things go a lot smoother.

Cheers.

ghost commented 9 years ago

Holy crap, I just reread our back and forth; I'm an internet douche... I want to apologize for my behaviour. I have no idea what sparked this in me. Your project is pretty freaking awesome and I really hope my argent comments didn't upset you enough to resent your work. I've been avoiding reading your reply. Dude, you're a a better man than I. I'm sorry.