Re-design Dredd configuration handling #1311

artem-zakharchenko · 2019-04-03T14:30:05Z

🚀 Why this change?

To have a configuration handling as a composition of utility functions
Removes deprecation warnings and drops support for the next CLI options: -c, -q/--silent, -l/--level, -t/--timestamp, -b/--sandbox, --hooksData.

Change log

📝 Related issues and Pull Requests

Relates to Possible problem in configuring apiary reporter by dredd.yml #612

✅ What didn't I forget?

To write docs
To adjust test suits
To put Conventional Changelog prefixes in front of all my commits and run npm run lint

artem-zakharchenko · 2019-04-03T14:32:05Z

I need to check Dredd's docs for references to the dropped CLI options. Will mark the respective checkbox in the PR when it's done.

Update: Checked the docs, references to the deprecated options have not been found. Neither the reference to the NodeJS version.

honzajavorek

I like the changes, only I did some more thinking about the deprecation warnings and I wonder whether it would make sense to literally break Dredd's behavior with this, i.e. to error and exit on the deprecated options. That seems actually more correct than to remove the warnings but to still accommodate the old (values of the) options.

That means, instead of warnings or removing warnings, we would turn them into errors and remove any coercing:

if (config.options.level) {
  errors.push('REMOVED: ...');
}
if (config.options.l && !['silent', 'error', 'warning', 'warn', 'debug'].includes(config.options.l)) {
  errors.push('REMOVED: ...');
}

We would also keep the existing errors, such as the one for hooksData. What do you think about such approach? I'm sorry for proposing changes to the task after it's done, but it occurred to me only when I had a calm moment to stare to the diff here.

artem-zakharchenko · 2019-04-04T11:38:50Z

Makes sense, @honzajavorek.

Then I will take this opportunity to adjust how validation and normalization of the config is performed currently. I would like to refrain from mutating the config object, and instead produce the next "resolved" config.

honzajavorek · 2019-04-04T11:52:46Z

Go for it 🚀

honzajavorek · 2019-04-04T12:59:59Z

Heads up - I found bug in how the inlined API descriptions are being loaded in the config. This is a fix: be36252

The apiDescriptions property gets accidentaly overriden by coerceRemovedOptions(), while it should be the other way around.

I have integration tests for API descriptions loading deep inside #1289 as well, but unfortunately they're dependent on other changes.

honzajavorek

I think it is on good path. The ramda code reads surprisingly well even for me who's not that skilled in it. Consider the behavior I pointed out in a comment. I don't think we need to implement all of it now, but we should get closer to it with the changes in this PR.

lib/configuration.js

test/unit/configuration-test.js

artem-zakharchenko · 2019-04-05T14:19:57Z

Updated the pull request with coercion of options.

It looks somewhat complicated at certain places, I will go through simplifying it after all tests pass. There are a few other options not migrated (i.e. c as a string value), and we can discuss whether we need something like coerceWithWarning to differentiate deprecated and removed options.

In a nutshell, I suggest to divide configuration options into three groups:

Supported options.
Deprecated options. Implying their usage will get deprecated in the next major release. Optional coercion may be performed for such options to prevent internals from using them.
Removed options. No coercion is performed, the usage of removed options produces an error and exits the process.

Removed options are omitted from the config object prior to any transformation, making the option design process more strict (you can't coerce removed option, maybe you mean deprecation).

I will focus on finishing the coercion and rewriting the validation function next week.

artem-zakharchenko · 2019-04-05T14:25:20Z

lib/configuration.js

+const coerceApiDescriptions = R.compose(
+  mapIndexed((content, index) => ({
+    location: `configuration.apiDescriptions[${index}]`,
+    content: R.when(R.has('content'), R.prop('content'), content),


I think we had an issue in the previous implementation that we always mapped over the apiDescriptions after coercing them to array. In case api descriptions already were an array, that would produce a nesting of their values.

dredd/lib/configuration.js

Lines 191 to 196 in 582f252

// Transform apiDescriptions from strings to an array of objects

outConfig.apiDescriptions = coerceToArray(inConfig.apiDescriptions)

.map((apiDescription, i) => ({

location: `configuration.apiDescriptions[${i}]`,

content: apiDescription,

}));

I've introduced a check if individual entry of descriptions is already properly formatted then output it as-is.

The intention is, externally, I'd like Dredd to have this as a plain array of strings:

["... content ...", "... content ..."]

Internally, I'd like it to be a more complex structure, an array of objects:

[ { content: "...", location: "...", ... }, { content: "...", location: "...", ... }, ]

The location is assigned property, user cannot specify it. When the API descriptions are provided inlined in the configuration of the Dredd class, it is assigned to configuration.apiDescriptions[original array index]. If the API descriptions come from files or URLs, the location is set to foobar.apib or http://example.com/foobar.apib. I can imagine having apiDescriptions supported in dredd.yml as well, so perhaps we could have dredd.yml like this:

apiDescriptions: - "... content ..." - "... content ..."

I think there's no reason to accept the more complex, internal structure on the input. If anything else than array of strings (or, for that matter, a single string) comes on the input, Dredd should error as misconfigured. I don't think that was the case currently, but if we're fixing this, I'd restrain from supporting the complex structure on input.

I don't see my above comments addressed (we're accommodating the array if it's already with location, but I think we should reject it in such case; valid inputs are a single string or an array of strings).

artem-zakharchenko · 2019-04-08T08:45:05Z

Finished with rewriting the coercion logic. Tests pass 🎉 (related tests, I mean).
Proceeding with the validation part.

artem-zakharchenko · 2019-04-09T14:07:36Z

Notes

R.mergeDeep leaves out object prototypes, thus be cautious when deeply merging objects with class instances as values (i.e. { emitter: new EventEmitter() }).
TransactioRunner and Hooks rely on as-is passed instance of Dredd configuration. However, during unit tests they are given the config manually, making any Dredd-level transformations irrelevant.

honzajavorek

Good job! I made a few comments. Some of them are nitpicks, some of them are to discuss. Some more remarks:

I'm missing more unit tests for the configuration.js functions. E.g. a unit test for the function taking care of the --user to token header, etc.
I'm missing at least one integration test to verify the old way still works. We've replaced options everywhere, also in tests, which means nothing in the test suite actually tries to feed Dredd with nested options and we don't know whether it works as expected.
Is the PR description still valid? I think it should be updated as we pivoted from the original purpose of the PR.
Users are given a warning that the options are now deprecated, but I see no information what should they do to prevent the warning and I see zero changes in the docs.
I don't know what's the difference between // TODO and @todo, but let's use just one.
I noticed you prefer compose over pipe. I kind of grapple with the fact it reverses the order of reading the code, but I guess that's just a matter of preference and habits.
Would it make sense to git pull --rebase origin master the branch so we get rid of the Merge branch ... commits?

lib/configuration.js

honzajavorek · 2019-04-11T11:49:49Z

lib/configuration.js

+const coerceApiDescriptions = R.compose(
+  mapIndexed((content, index) => ({
+    location: `configuration.apiDescriptions[${index}]`,
+    content: R.when(R.has('content'), R.prop('content'), content),


I don't see my above comments addressed (we're accommodating the array if it's already with location, but I think we should reject it in such case; valid inputs are a single string or an array of strings).

lib/configuration.js

test/integration/response-test.js

test/unit/Dredd-test.js

lib/configuration.js

test/unit/configuration-test.js

artem-zakharchenko · 2019-04-11T14:01:23Z

Thanks for review, @honzajavorek. I will address your comments and provide the changes.

Unit tests for granular configuration utils will be added.
Actually, we’ve removed the options nesting internally, but for tests I need to make sure we preserve the nesting to ensure that it won’t break anything for users who still use nested options.
Pull request description updated.
Will update the docs.
Sorry, let’s just use @todo if you don’t mind.
Apart from being a preference, compose is closer to the mathematical functional composition. It also more comfortable to read and apply functions right-to-left, since currying also works RTL by design. I would choose compose over pipe.

honzajavorek · 2019-04-11T14:16:15Z

Perhaps bikeshedding, but is @todo a JS Doc spec or something? I kind of find it hard to use for oneliners, like

const foo; // TODO: just this small thing

Also, there might already be some // FIXME:-s somewhere in the codebase. I don't think this is important though and I would never block the PR because of it. We can discuss this separately and then change it everywhere in Dredd to the one we choose, in a separate PR, so it's consistent.

honzajavorek

Looks good, I found just nitpicks mostly and a few things perhaps worth addressing, but not really anything serious.

lib/configuration/applyConfiguration.js

lib/configuration/normalizeConfig.js

honzajavorek · 2019-04-12T12:00:38Z

lib/configuration/applyConfiguration.js

+const defaultConfig = {
+  http: {},
+  server: null,
+  // emitter: new EventEmitter(), // TODO Merge + preserve prototype?


Nitpick: When I think about it, I'd appreciate if the comment was more verbose. In case we keep it here for a while, it might get murky when we forget the context. Don't be afraid to overcomment in this case:

const defaultConfig = { http: {}, server: null, // TODO Merge + preserve prototype? // // If the next line was uncommented, the subsequent merging machinery wouldn't be able to deal with // the fact the EventEmitter isn't a plain object. We worked around it by making the code to treat the event emitter // as a special case, but a more robust solution would be nicer. // // emitter: new EventEmitter(), ...

Just an example, perhaps it could be more brief 😄 I'm sometimes way too verbose with my writing.

In case of weird things, special cases, work arounds, etc., I'm all for writing tomes of wisdom for the next generations of readers into the code. Otherwise the suffering you went through to discover and solve the problem will be forgotten and someone else in the future will have to go through it again.

Yes, good call. I've provided detailed description below, where we explicitly set emitter after merging with default config. I will mention it here as well.

lib/configuration/applyConfiguration.js

honzajavorek · 2019-04-12T12:06:43Z

lib/configuration/applyConfiguration.js

+const validateConfig = require('./validateConfig');
+const { normalizeConfig } = require('./normalizeConfig');
+
+const defaultConfig = {


I wonder if this shouldn't be called DEFAULT_CONFIG so it's clearer in the code below this is a global static thing rather than a local variable

No hard opinion on that :) We are not reusing it anywhere, so it's not global per say, it remains scoped to this module.

We're exporting it and using it in the respective test file and I think the constant-like letter casing would convey better what the variable represents

test/integration/configuration/resolveConfig-test.js

test/unit/configuration-test.js

test/unit/configuration/normalizeConfig-test.js

honzajavorek · 2019-04-12T12:41:23Z

test/unit/configuration/normalizeConfig-test.js

+          });
+        });
+
+        it('with both "data" and "apiDescriptions"', () => {


lib/configuration/applyConfiguration.js

test/integration/configuration/resolveConfig-test.js

honzajavorek

Done reviewing the latest changes. Please address my comments and add the short options handling. Then I think we're good to go.

honzajavorek · 2019-04-12T16:19:04Z

lib/configuration/applyConfiguration.js

-  },
-};
+applyConfiguration.resolveConfig = resolveConfig;
+applyConfiguration.DEFAULT_CONFIG = DEFAULT_CONFIG;


I see your point about collisions with builtin function properties. What we did in other files was underscore dangling so it never happens:

applyConfiguration._resolveConfig = resolveConfig; applyConfiguration._DEFAULT_CONFIG = DEFAULT_CONFIG;

I wonder whether it's needed or not. Looking at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function I don't think we're likely to hit any of the prototype properties 🤔 Maybe we could remove the dangling from other files then (if you agree, we could do it in a separate PR and then we can remove https://github.com/apiaryio/dredd/blob/master/.eslintrc.js#L22-L25 as well).

docs/usage-cli.rst

docs/usage-js.rst

honzajavorek · 2019-04-26T16:04:48Z

I see all my comments addressed. What's the state of the short options handling? Otherwise I think it's good to go 🚀

One nitpick, if you want to scope a commit, the parentheses are before the colon:

- test: (resolveConfig) breaks down options test suits 
+ test(resolveConfig): breaks down options test suits

This is optional, in Dredd there is no convention around scoping the conventional changelog commits.

Another nitpick, it would be nicer to rebase the branch and squash/drop stuff not to have the __pycache__ in the git history at all.

artem-zakharchenko · 2019-04-29T12:34:23Z

Comments addressed, handling of options aliases provided. I will rebase to get rid of __pycache__, and it should be ready to merge.

artem-zakharchenko · 2019-04-29T13:45:13Z

Marked @todo notes now include links to the GitHub issues associated with them.

honzajavorek

Great work! 🚀 I found a few loose ends, but nothing which would block the PR:

default for the server option is missing - Re-design Dredd configuration handling #1311 (comment)
do we want to remove underscores dangling? (perhaps a separate issue and definitely a separate PR) Re-design Dredd configuration handling #1311 (comment)
@todo vs // TODO, we should use just one Re-design Dredd configuration handling #1311 (comment)
the test: (resolveConfig) breaks down options test suits commit format Re-design Dredd configuration handling #1311 (comment)
the issues created should link back to this PR for a reference (I did this right now)

Neither one of them is serious in any way and can be discussed and addressed later (or never). I'd focus on getting this merged now.

ApiaryBot · 2019-04-30T09:00:16Z

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

artem-zakharchenko requested a review from honzajavorek April 3, 2019 14:30

artem-zakharchenko force-pushed the cli-redesign branch from ec80419 to a717352 Compare April 3, 2019 14:31

artem-zakharchenko changed the title ~~Re-designs CLI options composition~~ Drops NodeJS v6 support. Removes warnings on deprecated Dredd CLI options. Apr 3, 2019

honzajavorek reviewed Apr 3, 2019

View reviewed changes

honzajavorek mentioned this pull request Apr 4, 2019

Update make-dir to the latest version 🚀 #1300

Merged

honzajavorek reviewed Apr 4, 2019

View reviewed changes

lib/configuration.js Outdated Show resolved Hide resolved

test/unit/configuration-test.js Outdated Show resolved Hide resolved

artem-zakharchenko commented Apr 8, 2019

View reviewed changes

artem-zakharchenko changed the title ~~Drops NodeJS v6 support. Removes warnings on deprecated Dredd CLI options.~~ Re-design Dredd configuration handling Apr 8, 2019

artem-zakharchenko force-pushed the cli-redesign branch 3 times, most recently from 46b5f5e to 0561654 Compare April 8, 2019 11:43

artem-zakharchenko force-pushed the cli-redesign branch from bfa0a68 to 08e8fbf Compare April 10, 2019 11:48

artem-zakharchenko requested a review from honzajavorek April 10, 2019 13:07

honzajavorek suggested changes Apr 11, 2019

View reviewed changes

honzajavorek reviewed Apr 12, 2019

View reviewed changes

artem-zakharchenko force-pushed the cli-redesign branch from 09b4bdd to 56c4600 Compare April 12, 2019 13:02

honzajavorek reviewed Apr 12, 2019

View reviewed changes

lib/configuration/applyConfiguration.js Show resolved Hide resolved

test/integration/configuration/resolveConfig-test.js Outdated Show resolved Hide resolved

test/integration/configuration/resolveConfig-test.js Outdated Show resolved Hide resolved

honzajavorek suggested changes Apr 12, 2019

View reviewed changes

artem-zakharchenko force-pushed the cli-redesign branch from 0eea4cf to 0006046 Compare April 18, 2019 14:25

artem-zakharchenko added 6 commits April 29, 2019 09:52

refactor: splits coercing compositions into smaller functions

3be690d

test: assert for falsy values in level coercion

517ee20

refactor: add explanatory comment upon configuration emitter

3da5bfc

test: asserts instance of config emitter, cleans up tests

6e87d56

refactor: renames configuration root option "server" to "endpoint"

fc74d99

refactor: uses single default export of modules, injects utils

82e6956

artem-zakharchenko force-pushed the cli-redesign branch 3 times, most recently from d183e5c to 758add3 Compare April 29, 2019 12:16

artem-zakharchenko added 2 commits April 29, 2019 14:54

docs: adds configuration "options" changes

cc6281f

docs: updates cli and js usage to include proper tags

52ff0ab

artem-zakharchenko force-pushed the cli-redesign branch from 758add3 to db1127f Compare April 29, 2019 12:55

honzajavorek approved these changes Apr 29, 2019

View reviewed changes

This was referenced Apr 29, 2019

Remove configuration object flattening #1344

Open

Deep merge default and custom config preserving prototypes #1345

Open

Simplify logging options application #1346

Open

artem-zakharchenko force-pushed the cli-redesign branch from 6a05df5 to d97489f Compare April 30, 2019 08:05

artem-zakharchenko added 3 commits April 30, 2019 10:06

test(resolve-config): breaks down options test suits

84694ef

refactor: handles option alias during config validation

6261639

refactor: adds links to github issues of todo notes

adf723a

artem-zakharchenko force-pushed the cli-redesign branch from d97489f to adf723a Compare April 30, 2019 08:06

refactor: aligns "TODO" annotations usage

d10b77b

artem-zakharchenko merged commit bdef901 into master Apr 30, 2019

artem-zakharchenko deleted the cli-redesign branch April 30, 2019 08:53

ApiaryBot added the released label Apr 30, 2019

artem-zakharchenko mentioned this pull request Apr 30, 2019

Possible problem in configuring apiary reporter by dredd.yml #612

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-design Dredd configuration handling #1311

Re-design Dredd configuration handling #1311

artem-zakharchenko commented Apr 3, 2019 •

edited

Loading

artem-zakharchenko commented Apr 3, 2019 •

edited

Loading

honzajavorek left a comment •

edited

Loading

artem-zakharchenko commented Apr 4, 2019

honzajavorek commented Apr 4, 2019

honzajavorek commented Apr 4, 2019

honzajavorek left a comment

artem-zakharchenko commented Apr 5, 2019 •

edited

Loading

artem-zakharchenko Apr 5, 2019

honzajavorek Apr 9, 2019

honzajavorek Apr 11, 2019

artem-zakharchenko commented Apr 8, 2019 •

edited

Loading

artem-zakharchenko commented Apr 9, 2019 •

edited

Loading

honzajavorek left a comment •

edited

Loading

honzajavorek Apr 11, 2019

artem-zakharchenko commented Apr 11, 2019

honzajavorek commented Apr 11, 2019

honzajavorek left a comment

honzajavorek Apr 12, 2019

artem-zakharchenko Apr 12, 2019

honzajavorek Apr 12, 2019

artem-zakharchenko Apr 12, 2019

honzajavorek Apr 12, 2019

honzajavorek Apr 12, 2019

honzajavorek left a comment

honzajavorek Apr 12, 2019

honzajavorek commented Apr 26, 2019 •

edited

Loading

artem-zakharchenko commented Apr 29, 2019

artem-zakharchenko commented Apr 29, 2019

honzajavorek left a comment •

edited by artem-zakharchenko

Loading

ApiaryBot commented Apr 30, 2019

	// Transform apiDescriptions from strings to an array of objects
	outConfig.apiDescriptions = coerceToArray(inConfig.apiDescriptions)
	.map((apiDescription, i) => ({
	location: `configuration.apiDescriptions[${i}]`,
	content: apiDescription,
	}));

Re-design Dredd configuration handling #1311

Re-design Dredd configuration handling #1311

Conversation

artem-zakharchenko commented Apr 3, 2019 • edited Loading

🚀 Why this change?

Change log

📝 Related issues and Pull Requests

✅ What didn't I forget?

artem-zakharchenko commented Apr 3, 2019 • edited Loading

honzajavorek left a comment • edited Loading

Choose a reason for hiding this comment

artem-zakharchenko commented Apr 4, 2019

honzajavorek commented Apr 4, 2019

honzajavorek commented Apr 4, 2019

honzajavorek left a comment

Choose a reason for hiding this comment

artem-zakharchenko commented Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zakharchenko commented Apr 8, 2019 • edited Loading

artem-zakharchenko commented Apr 9, 2019 • edited Loading

Notes

honzajavorek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zakharchenko commented Apr 11, 2019

honzajavorek commented Apr 11, 2019

honzajavorek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honzajavorek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honzajavorek commented Apr 26, 2019 • edited Loading

artem-zakharchenko commented Apr 29, 2019

artem-zakharchenko commented Apr 29, 2019

honzajavorek left a comment • edited by artem-zakharchenko Loading

Choose a reason for hiding this comment

ApiaryBot commented Apr 30, 2019

artem-zakharchenko commented Apr 3, 2019 •

edited

Loading

artem-zakharchenko commented Apr 3, 2019 •

edited

Loading

honzajavorek left a comment •

edited

Loading

artem-zakharchenko commented Apr 5, 2019 •

edited

Loading

artem-zakharchenko commented Apr 8, 2019 •

edited

Loading

artem-zakharchenko commented Apr 9, 2019 •

edited

Loading

honzajavorek left a comment •

edited

Loading

honzajavorek commented Apr 26, 2019 •

edited

Loading

honzajavorek left a comment •

edited by artem-zakharchenko

Loading