-
Notifications
You must be signed in to change notification settings - Fork 146
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
Contributions, configs, building #75
Comments
Simplification is a worthwhile goal, I agree. It would be nice to get rid of the double configuration system and come up with a better way to configure the tool for the NYTimes and other organizations. We don't customize the script at all. To deploy it, we just copy ai2html.js from the repo to the Illustrator Scripts directory. To decide which settings to apply, the script currently looks for NYT-specific fonts to tell if it's running in a Times-specific context, then looks for files and directories that are specific to our CMS. It's crude but it seems to work ok. You are proposing that we organize the code into a collection of modules that would be bundled together by a build tool, is that right? Have you thought about how the modules might be organized? Do you have thoughts about which module system or tooling would be a good choice for this project? It also sounds like you're envisioning that organization-specific configuration would be done at build time and 'baked in' to the executable. I'm not sure how that would work in practice, can you give some more details? Looking forward to hearing your suggestions. |
Perhaps we just want a |
Thats good to know how you deploy it. I was thinking something really simple for a build system, like concatenation. The JS engine in Illustrator is not well documented, so I'd be worried about using clever build systems that assume node or v8 or popular browsers. Should be pretty easy to accomplish with We could adjust that script to output different versions with different config sections. Happy to put together a proof of concept for you to take a look at. |
Hi @ryanmark I like Tom's suggestion of switching to an external settings file (and a single set of default settings). I'm not sold on splitting up ai2html.js into pieces that could be mixed-and-matched. It's simpler for us not to have any build process, and easier to support the script if everyone is running the same file. I'm happy to reconsider, though, if you want to put together a proof of concept. --mbloch |
Here is a proof of concept: https://github.com/voxmedia/ai2html/tree/build-system My intention in breaking up the large file is not to make pieces that can be mixed and matched, but to attempt to reduce the amount of code you have to look through at one time to understand the flow. I left the bulk of the original ai2html.js together in The build script takes json files from I changed the script extension to This branch won't work in it's current state, it'll require some adjustments. Just showing this as an idea to get feedback. If y'all like it, I'll make it functional. With this approach, a new user can grab this repo, copy |
Thanks for this, I'll have a look and get back to you with notes... it might be a few days. |
Curious to know if this has progressed in any way. @mbloch, did you have time to take a look? |
Hi! following up on this. Any thoughts on the code I prepared above, @mbloch ? |
I moved this comment to a separate issue, #86 |
Some comments on your sample code, @ryanmark Your build script addresses two problems with the current program: configuration and code readability. These are good areas to work on, for sure. Thank you for spending time on this :) I feel that configurability is the more pressing issue. Moving the config options into external JSON files as you've done is an improvement. I think we can go even further (I moved my suggestions to a separate issue, #86). If ai2html switches to YAML for configuration, we might want to either bake out versions of the script with different configurations included -- like what you've done -- or load configuration settings from external files at run time. Code readability is important too, certainly. You put most of the code from the original script into the file "functions.js" (2626 lines), so still a big chunk of code. As you mentioned, "functions.js" could be split up into smaller files, but then contributors would be searching between multiple files to trace the flow of the program, which doesn't seem any easier to me than searching inside a single file. One improvement might be to assign all of the functions in one source code file to the same namespace object, so for example, to call the "map" function from the file "utils.js" you would go I'm already familiar with the current script, so I'm not the best one to judge if these changes would make the code much easier to follow. I hope some other people will weigh in. |
I love the idea of having a runtime config file, it would make it much easier for anyone to take and use ai2html. However I could see how that might be more difficult for NYT folks to have to install the plugin and a config file. Thoughts about code organization: Source control works best with many files. More, smaller files means cleaner merges and fewer merge conflicts, and probably (conjecture warning) more contributions and PRs, fewer forks. As someone who reads lots of other peoples code and I find it much much easier to understand when they organize their code into different files that are labeled usefully. Just like with an article, I should be able to get the gist of the thing right away and follow the signposts to read more about what i'm interested in. When all the code is in a single file, there are no signposts and the lead is buried. I can't just start reading the code, I have to scan it all in order to find the most important part, then build my own mental model of the whole thing before I can track down the parts I'm interested in. Breaking the code out into other files, even if it's something generic like Breaking out the code also helps to hide stuff that few people will ever have to look at. The minified json2 library for example. Almost nobody will ever have to touch or look at that. Standard library functions like This is just my 2 cents about organizing code, which I'll admit I love to do. There are lots of forks of this repo and seemingly many other news orgs using it. Would love to see us find a way to funnel all that effort into a single shared project so we can all benefit. |
Oh and regarding name-spacing functions - I think that can be a great idea, but does not address the concerns about reading and contributing that breaking up the single file does. |
Hi @ryanmark, If you're saying that namespacing is not relevant to code readability, I guess I disagree. If I see that a function is being called from a module namespace, then I know where to look to find the function's definition. And if I see a bare function call, it's useful if I can assume that the function is local to the current source code file. I would prefer organizing the code into proper modules (with their own internal scope) over copying a bunch of bare functions into the main body of the script. Your other points are well taken. (I hope that the configuration changes I'm pushing for will remove the need for many orgs to maintain their own versions of the project -- currently you need to fork just to add your own fonts.) I'm happy to keep discussing the code organization issue. Based on an unscientific survey, I'd say that my colleagues are still not on board. Cheers, mbloch |
I totally agree with your point about name spacing. It can improve readability. I was saying however, that if you just namespace functions but keep all the code in one file, it does not address the other points I made above. I still have to look at and understand most the code in order to find the parts I'm looking for. Thank you for your considerations. I will look forward to the changes to improve configurations and in the meantime keep our changes in a fork, like other folks. |
I see, we misunderstood each other... I meant to suggest using namespacing together with splitting the script into separate files. I'd like to get the configuration changes done first, then revisit how to organize the code better. |
For followers of this issue, I've made some changes to ai2html configuration, discussed here: #86 |
I'm curious in simplifying the config. In my branch I removed the multiple configs, which was really confusing when I first tried to use ai2html.
Does the NYT customize the script before giving it to folks? Or do yall need for the ai2html.js file in this repo to work out of the box?
I'm also curious if yall are interested in breaking this file up and adding a script to build it. It would make it much easier to work in this code and would also make it possible to build different versions for NYT and other organizations.
The text was updated successfully, but these errors were encountered: