Skip to content

Conversation

@tjprescott
Copy link
Member

@tjprescott tjprescott commented May 31, 2018

image

Closes #5954. Closes #5968. Closes #6059.

Deprecates (hides) the monitor autoscale-settings commands as well as the previously deprecated commands in redis: import-method, update-settings, list-all, and the patch-schedule patch-schedule group.

To be done:

  • Test extensions
  • Ref doc gen script

This checklist is used to make sure that common guidelines for a pull request are followed.

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

@troydai
Copy link
Contributor

troydai commented Jun 4, 2018

Please take a look at the CI.

@troydai
Copy link
Contributor

troydai commented Jun 4, 2018

Many versions in setup.py will need to be updated once the 2.0.34 is released to the PyPI.

@tjprescott tjprescott modified the milestones: Sprint 39, Sprint 40 Jun 12, 2018
@codecov-io
Copy link

codecov-io commented Jun 13, 2018

Codecov Report

Merging #6472 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##           dev   #6472   +/-   ##
===================================
  Coverage    0%      0%           
===================================
  Files       11      11           
  Lines      145     145           
  Branches    11      11           
===================================
  Misses     145     145

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5c43c8...863758d. Read the comment docs.

@tjprescott tjprescott changed the title [DNM] Deprecation strategy Add Deprecation support Jun 19, 2018
@tjprescott
Copy link
Member Author

tjprescott commented Jun 19, 2018

Per discussion with @mayurid and @twitchax we will not worry about interactive support for deprecation messages in this PR.

@tjprescott
Copy link
Member Author

@williexu @troydai @yugangw-msft please take a look.

@williexu
Copy link
Contributor

@tjprescott I'm assuming interactive won't break when encountering deprecated items, at least.

Out of curiosity, what is the behavior for deprecated commands/args/etc.
Are they just treated like normal ones?

@tjprescott
Copy link
Member Author

Yes, I made sure interactive didn't crash. They run just like normal.

@tjprescott tjprescott modified the milestones: Sprint 40, Sprint 41 Jun 25, 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also delete the groups from the loader's command_group_table here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

This return will be reached, probably intended to return the list if there are options in it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was dead code. Removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the switch from using get_parameter_options?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. Reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the command_table and command_group_table as hidden fields, or let them be accessed from the loader? They should not be exposed easily. These properties should just have the command/group names, which can be referenced from the table keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed them to just return the keys. Now I use the get_metadata methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these get-metadata methods used anywhere?

Copy link
Member Author

@tjprescott tjprescott Jun 25, 2018

Choose a reason for hiding this comment

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

No, though it seems they do what you were asking for above. Now that the commands and command_groups properties just return keys, I use this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we no longer limiting the kwargs being merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of raising an error (some unsupported kwargs may be in a kwarg bundle), on the next line we simply gather up the supported args.

Copy link
Contributor

Choose a reason for hiding this comment

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

travis, can you explain this? was this moved from somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was copied from the base knack implementation and essentially cuts out where the old-style deprecation messages were displayed in knack. In the future I could remove that code (potentially) from knack and then just rely on the base implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't use this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

show_help reverted to the knack implementation. print_detailed_help was changed at the knack and az level to be a regular method, not a class method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what the bug was that caused this to be needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is just ensuring that all help will end in a proper "stopping" punctuation mark.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't be necessary and works just fine without this on Python 3. But for whatever reason, stupid Python 2.7 ends up looking into the SDK docstring, finds some non-ASCII character, and crashes. Not wanting to invest a lot of time into the philosophical question of why Python 2.7 is such crap, I just went with this.

Copy link
Contributor

@williexu williexu Jun 25, 2018

Choose a reason for hiding this comment

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

These probably should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. Debugging remnants.

@tjprescott
Copy link
Member Author

@williexu comments addressed. Please take another look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm seeing this wrong but are you deleting all the groups in the table...?
It should only be the ones from the selected modules/extensions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you're right. Fixed. However, this will not exclude orphan help file entries (botservice has some).

Copy link
Contributor

Choose a reason for hiding this comment

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

logging.warning will call str on the argument for you

Copy link
Contributor

Choose a reason for hiding this comment

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

is it certain that args will have at least one element?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It is the same in knack.

Copy link
Contributor

@williexu williexu Jun 26, 2018

Choose a reason for hiding this comment

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

This is inside a for loop over all the commands in the command_table.
Please move this outside the for loop

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thought I had done that!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

8 participants