Skip to content

Add warnings for conflicting/deprecated component attribs - #1057

Merged
yamgent merged 2 commits into
MarkBind:masterfrom
nbriannl:1045-component-warnings
Mar 8, 2020
Merged

Add warnings for conflicting/deprecated component attribs#1057
yamgent merged 2 commits into
MarkBind:masterfrom
nbriannl:1045-component-warnings

Conversation

@nbriannl

@nbriannlnbriannl commented Feb 26, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Other, please explain: Add warning for bad markbind attribute usage

Fixes#1048

What is the rationale for this request?

Discussion here MarkBind/vue-strap#118 (comment)

Warning the user at compile time of the usage of conflicting/deprecated attributes in components is good for author usability.

What changes did you make? (Give an overview)

When parsing component attributes, check for presence of inconsistent, and deprecated attributes and warn accordingly.

Used console.log instead of logger utility because parser.test.js fails. This might possibly be a bug regarding how logger.js resolves the directory. See #1060

No documentation needed since it is a warning on the CLI side.

Provide some example code that this change will affect:

The code in this gist will now produce warnings
https://gist.github.com/nbriannl/5c44b3ee8a8af78c45a7a4f3659ce196

Is there anything you'd like reviewers to focus on?
Instead of writing another method to warn for deprecated slot name (which is incurring an addtional 1 pass search on the element's children), I wanted to change renameSlot to processSlot which will do both checking for deprecated slot name, and renaming. But this assumes that every rename of a slot is for a deprecated name. Therefore, I stuck with making two separate functions.

Testing instructions:
markbind serve the example code in the gist.
https://gist.github.com/nbriannl/5c44b3ee8a8af78c45a7a4f3659ce196

You should see
image

Proposed commit message: (wrap lines at 72 characters)

Add warnings for conflicting/deprecated component attribs

nbriannl
nbriannl commented Feb 26, 2020
View reviewed changes
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
@nbriannlnbriannl mentioned this pull request Feb 27, 2020
Closed
nbriannl
nbriannl commented Feb 27, 2020
View reviewed changes
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
@nbriannlnbriannl changed the title [WIP] Add warnings for inconsistent/deprecated component attribsAdd warnings for inconsistent/deprecated component attribsFeb 27, 2020
ang-zeyu
ang-zeyu reviewed Feb 27, 2020
View reviewed changes

@ang-zeyuang-zeyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Just some nits:

Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
openorclose
openorclose requested changes Feb 28, 2020
View reviewed changes

@openorcloseopenorclose left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use console.warn for now since they are warnings? Then we can change back to logger once it's working.

Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
openorclose
openorclose reviewed Feb 28, 2020
View reviewed changes
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
@nbriannlnbriannl changed the title Add warnings for inconsistent/deprecated component attribsAdd warnings for conflicting/deprecated component attribsFeb 28, 2020
marvinchin
marvinchin requested changes Feb 28, 2020
View reviewed changes

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see a more structured way of flagging errors to the user :) Added a couple of suggestions to make these logging functions more versatile.

Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
Comment threadsrc/lib/markbind/src/parsers/componentParser.js Outdated
marvinchin
marvinchin approved these changes Mar 4, 2020
View reviewed changes

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yamgentyamgent added this to the v2.11.1 milestone Mar 7, 2020
@yamgent

yamgent commented Mar 7, 2020

Copy link
Copy Markdown
Member

@nbriannl resolve the conflicts

@nbriannl
nbriannlforce-pushed the 1045-component-warnings branch from 0e04800 to f9e2e31CompareMarch 7, 2020 07:30
@nbriannl
Add warnings for inconsistent/deprecated component attribs
2e3a45b
Use console log instead of logger
add warning for modal slots
Rewrite more efficient methods and use element
Rewrite docs and reduce obvious code
Reduce obvious code
Remove empty lines
Add gaurd clause and remove excessive todo
Change console log to warn and remove return
Change console log to warn
Add more warns for dropdown
Use conflicting instead of inconsistent
Change c to child
Add note regarding conlifcting attribs
Restructure warnDeprecatedAttributes
Use Foreach instead of loop
Group all warns and put at start of parsing methods
Restructure warnDeprecatedSlotName
@nbriannl
linting
de67b19
@nbriannl
nbriannlforce-pushed the 1045-component-warnings branch from f9e2e31 to de67b19CompareMarch 7, 2020 08:00
@yamgent
yamgent merged commit ba79813 into MarkBind:masterMar 8, 2020
@yamgentyamgent added the pr.Enhancement 📈 Enhancement to an existing feature label Mar 8, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 9, 2020
@Tejas2805
Merge branch 'master' ofhttps://github.com/MarkBind/markbind
31d0325
* 'master' of https://github.com/MarkBind/markbind:
2.12.0
Update outdated test files
Update vue-strap version to v2.0.1-markbind.37
Fix refactor to processDynamicResources (MarkBind#1092)
Implement lazy page building for markbind serve (MarkBind#1038)
Add warnings for conflicting/deprecated component attribs (MarkBind#1057)
Allow changing parameter properties (MarkBind#1075)
Custom timezone for built-in timestamp (MarkBind#1073)
Fix reload inconsistency when updating frontmatter (MarkBind#1068)
Implement an api to ignore content in certain tags (MarkBind#1047)
Enable AppVeyor CI (MarkBind#1040)
Add heading and line highlighting to code blocks (MarkBind#1034)
Add dividers and fix bug in siteNav (MarkBind#1063)
Fixed navbar no longer covers modals (MarkBind#1070)
Add copy code-block plugin (MarkBind#1043)
Render plugins on dynamic resources (MarkBind#1051)
Documentation for Implement no-* attributes for <box> (MarkBind#1042)
Migrate to bootstrap-vue popovers (MarkBind#1033)
Refactor preprocess and url processing functions (MarkBind#1026)
Add pageNav to Using Plugins Page (MarkBind#1062)
@Parcly-TaxelParcly-Taxel mentioned this pull request Mar 10, 2020
Merged
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
@nbriannlMarvin Chin
Add warnings for conflicting/deprecated component attribs (#1057)
fe53950
@nbriannl
nbriannl deleted the 1045-component-warnings branch April 16, 2020 09:55
@nbriannlnbriannl mentioned this pull request Apr 19, 2020
Merged
nbriannl added a commit that referenced this pull request May 7, 2020
@nbriannl
Replace console with logger (#1207)
80b4481
Replace console with logger
A follow up to #1057.
Let us also remove the error handler instance var
And directly use logger instead
@jonahtanjzjonahtanjz mentioned this pull request Feb 13, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marvinchinmarvinchinmarvinchin approved these changes
@yamgentyamgentAwaiting requested review from yamgent
@le0tanle0tanAwaiting requested review from le0tan
@ang-zeyuang-zeyuAwaiting requested review from ang-zeyu
@openorcloseopenorcloseAwaiting requested review from openorclose

Assignees

No one assigned

Labels

pr.Enhancement 📈Enhancement to an existing feature

Projects

None yet

Milestone

v2.12.0

Development

Successfully merging this pull request may close these issues.

Implement warning logging when inconsistent or deprecated attributes are used

5 participants

@nbriannl@yamgent@ang-zeyu@openorclose@marvinchin