Add config_overrides argument to initializer of MergerConfig #225
Conversation
|
@wwkimball Just pinging you to let you know I made the changes you requested. Do you want me to start writing tests for it? |
|
Given that the concrete behavior of the new argument is to override whatever may be in the external configuration file -- should one have been set -- please change the new property name from |
|
Besides that, please change the merge target to the development branch. There is normally a formal process for updating master that is tied to releases, so merges to master aren't allowed unless they come from development. |
|
The tests all pass; please resolve the kwargs matter and we'll push this along. |
| @@ -334,8 +346,15 @@ def _load_config(self) -> None: | |||
|
|
|||
| if config_file: | |||
There was a problem hiding this comment.
I just noticed the overrides will only apply when a config file has been provided. Wasn't the original goal to entirely avoid using an external config file for API users? Using an override mechanism can get us there, but only if it also handles the case of there being no config file to start with. Having said that, such a change blows open the scope of this change because the change would have to also cover the rest of the interface provided by ConfigParser (like .sections(), et al).
This feels like we're missing the goal. Will you be happy with the compromise you've added here or should we dig deeper into this to provide a solution that actually makes the config file optional?
There was a problem hiding this comment.
This is just a case of not fixing an indent after making a change. Everything should work just fine by un-indenting those lines because even if the config file doesn't exist, we still use ConfigParser, adding it as a class member only if we added the overrides to it (by using sections()).
Though your comment made me realize that my tests should have caught this earlier, so I checked out my tests and realized that anything specified in self.args has a higher priority than my changes, which I think misses the mark. I'll take a deeper look into fixing this when I have a bit more time to update everything, but I'll push some changes to the tests soon to illustrate the issue.
There was a problem hiding this comment.
Disregard that last part, turns out it works just fine based on the tests I added. I added a fix and some tests for the fix.
leviem1
changed the title
|
@wwkimball Bump |
|
@wwkimball Hope you had a good holiday weekend. Just wanted to bump this one last time. |
Thanks! After an unexpected layoff and finally getting hired again, I'm also working day and night with unfamiliar technologies at my new job trying to impress my new boss and company so "the new guy" (me) isn't first to be laid off should that happen again. The way things are going in the macroeconomy, it's a legitimate fear that is making me "workaholoc" my way through this new project. Please accept my apologies for letting this affect my responsiveness to your PR on this open source project. I've never forgotten about this. It's just that every time I boot up a machine, I can't help but delve into my new job. My plan is to run your branch locally to triple-check the tests on Windows and Mac (surprising differences do sometimes come up, particularly with Python default behaviors we take for granted, notably around UTF-8 and Newline handling). Once passing, I'll merge into development, update the CHANGES log, make sure none of my unrelated development work is lingering (a significant refactoring due to breaking changes in ruamel.yaml), and release your change with a minor (second number) version bump. Unless this is a blocker for your work (I assume you are running with your branch and are thus not blocked), I just feel like I need to get my work project to a happy place, first. If you are being blocked by my delay, please let me know and I'll reprioritize. |
|
Hey, no worries, I was recently unemployed for three months and now I'm in a weird limbo in a contract-to-hire position, so I definitely understand. I am currently using the workaround I mentioned in my issue #223, so I'm not blocked. Take your time, I just wanted to make sure this didn't fall off your radar. And good luck at your new position! |
|
@wwkimball Hey, just wanted to see how it's going. This isn't a bump, just want to see how you've been getting along with the new job |
I'm on a team of one working day and night plus weekends with no overtime pay while being paid 1/4 my former salary to write an ERP-light in technologies I've had to teach myself on-the-fly (Node.js, React.js, and Next.js) because my new employer has zero budget for IT but wants big-company tooling, anyway. It's both a thrill (I love learning) and a serious challenge (I may have bit off more than I can chew). I mistakenly told my boss I could write a simple purchase order system in two weeks. If I'd stuck with technologies I already knew (I originally imagined Linux-Apache-PHP-PostgreSQL) and never gave any demos where everyone wanted the software to do more and more (scope creep), sure; two weeks would have been enough. I'm now going on three months and people are already saying I'm not justifying my already meager salary (nothing to show for the time other than demos on my laptop). I'm desperately trying to develop a deployment pipeline (Docker-based to a tiny VM running on a tiny NAS without the luxury of Jenkins) for this project before Christmas. In a nutshell, it's rough. And in some ways, exciting. |
|
Yikes, sorry to hear that. It's always fun to learn and take on a new challenge, but I understand how it feels when you get stuck on a project that should've been straightforward. I know your situation is probably very different than my experiences, but I've found that more than anything it helps to just ship a turd early on and build from there. Nobody knows what they want, but they know what they don't. Plus, they want to see that stuff is "working", even if it does so poorly. Plus, it doesn't sound like they're paying for your best either. Sometimes you have to deliver what is asked, not what is your best. I hope your situation improves soon and I hope you had a great christmas! |
|
@leviem1 Sorry this took so long! Due to a minor holdup waiting for my employer's replacement server (ran the old one until it failed... 14 years!), I found some free time today. |
* Add config_overrides argument to initializer of MergerConfig (#225) Fix #223 * Update tests for pytest changes * Release preparation for 3.8.2 * Drop formal support for Python 3.6 * Mention Python 3.12 support is pending a dateutil release * Update GitHub Actions due to deprecation warnings * Update ruby gem path for new GH Action vers * Newer Ruby bin path requires more than just a version bump * Remove restriction on eyaml version (#231) * Fix version of gh-actions-pypi-publish * Split Coveralls.IO publication to resolve throttling * Fix coveralls workflow name * EYAML 2.x->3.x key incompatibility has been resolved --------- Co-authored-by: Levi Muniz <[email protected]>
Resolves #223 by passing configuration values through MergerConfig API instead of via configuration files.